ECPG cleanup and fix for clang compile-time problem
Here is a patch series that aims to clean up ecpg's preprocessor
code a little and fix the compile time problems we're seeing with
late-model clang [1]/messages/by-id/CAGECzQQg4qmGbqqLbK9yyReWd1g=d7T07_gua+RKXsdsW9BG-Q@mail.gmail.com. I guess whether it's a cleanup is in the eye of
the beholder, but it definitely succeeds at fixing compile time: for
me, the time needed to compile preproc.o with clang 16 drops from
104 seconds to less than 1 second. It might be a little faster at
processing input too, though that wasn't the primary goal.
The reason that clang is having a problem seems to be the large number
of virtually-duplicate semantic actions in the generated preproc.y.
So I looked for a way to allow most productions to use the default
semantic action rather than having to write anything. The core idea
of this patch is to stop returning <str> results from grammar
nonterminals and instead pass the strings back as Bison location data,
which we can do by redefining YYLTYPE as "char *". Since ecpg isn't
using Bison's location logic for error reports, and seems unlikely to
do so in future, this doesn't cost us anything. Then we can implement
a one-size-fits-most token concatenation rule in YYLLOC_DEFAULT, and
only the various handmade rules that don't want to just concatenate
their inputs need to do something different. (Within those handmade
rules, the main notational change needed is to write "@N" not "$N"
for the string value of the N'th input token, and "@@" not "$@"
for the output string value.) Aside from not giving clang
indigestion, this makes the compiled parser a little smaller since
there are fewer semantic actions that need code space.
As Andres remarked in the other thread, the parse.pl script that
constructs preproc.y is undocumented and unreadable, so I spent
a good deal of time reverse-engineering and cleaning that up
before I went to work on the actual problem. Four of the six
patches in this series are in the way of cleanup and adding
documentation, with no significant behavioral changes.
The patch series comprises:
0001: pgindent the code in pgc.l and preproc.y's precursor files.
Yeah, this was my latent OCD rearing its head, but I hate looking
at or working on messy code. It did actually pay some dividends
later on, by making it easier to make bulk edits.
0002: improve the external documentation and error checking of
parse.pl. This was basically to convince myself that I knew
what it was supposed to do before I started changing it.
The error checks did find some errors, too: in particular,
it turns out there are two unused entries in ecpg.addons.
(This implies that check_rules.pl is completely worthless and should
be nuked: it adds build cycles and maintenance effort while failing
to reliably accomplish its one job of detecting dead rules, because
what it is testing is not the same thing that parse.pl actually does.
I've not included that removal in this patch series, though.)
0003: clean up and simplify parse.pl, and write some internal
documentation for it. The effort of understanding it exposed that
there was a pretty fair amount of dead or at least redundant code,
so I got rid of that. This patch changes the output preproc.y
file only to the extent of removing some blank lines that didn't
seem very useful to preserve.
0004: this is where something useful happens, specifically where
we change <str>-returning productions to return void and instead
pass back the desired output string as location data. In most
cases the productions now need no explicit semantic action at all,
allowing substantial simplification in parse.pl.
0005: more cleanup. I didn't want to add more memory-management
code to preproc/type.c, where mm_alloc and mm_strdup have lived
for no explicable reason. I pulled those and a couple of other
functions out to a new file util.c, so as to have a better home
for new utility code.
0006: the big problem with 0004 is that it can't use the trick
of freeing input substrings as soon as it's created the merged
string, as cat_str and friends have historically done. That's
because YYLLOC_DEFAULT runs before the rule's semantic action
if any, so that if the action does need to look at the input
strings, they'd already be freed. So 0004 is leaking memory
rather badly. Fix that by creating a notion of "local" memory
that will be reclaimed at end of statement, analogously to
short-lived memory contexts in the backend. All the string
concatenation work happens in short-lived storage and we don't
worry about getting rid of intermediate values immediately.
By making cat_str and friends work similarly, we can get rid
of quite a lot of explicit mm_strdup calls, although we do have
to add some at places where we're building long-lived data
structures. This should greatly reduce the malloc/free traffic
too, at the cost of eating somewhat more space intra-statement.
In my view 0006 is about the scariest part of this, as it's
hard to be sure that there are no use-after-free problems
wherein a pointer to a short-lived string survives past end
of statement. It gets through the ecpg regression tests
under valgrind successfully, but I don't have much faith
in the thoroughness of the code coverage of those tests.
(If our code coverage tools worked on bison/flex stuff,
maybe this'd be less scary ... but they don't.)
I'll park this in the July commitfest.
regards, tom lane
[1]: /messages/by-id/CAGECzQQg4qmGbqqLbK9yyReWd1g=d7T07_gua+RKXsdsW9BG-Q@mail.gmail.com
Attachments:
v1-0001-Clean-up-indentation-and-whitespace-inconsistenci.patchtext/x-diff; charset=us-ascii; name*0=v1-0001-Clean-up-indentation-and-whitespace-inconsistenci.p; name*1=atchDownload+2609-2013
v1-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.patchtext/x-diff; charset=us-ascii; name*0=v1-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.p; name*1=atchDownload+120-64
v1-0003-Major-cleanup-simplification-and-documentation-of.patchtext/x-diff; charset=us-ascii; name*0=v1-0003-Major-cleanup-simplification-and-documentation-of.p; name*1=atchDownload+292-195
v1-0004-Re-implement-ecpg-preprocessor-s-string-managemen.patchtext/x-diff; charset=us-ascii; name*0=v1-0004-Re-implement-ecpg-preprocessor-s-string-managemen.p; name*1=atchDownload+752-1217
v1-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.patchtext/x-diff; charset=us-ascii; name*0=v1-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.p; name*1=atchDownload+107-90
v1-0006-Improve-ecpg-preprocessor-s-memory-management.patchtext/x-diff; charset=us-ascii; name=v1-0006-Improve-ecpg-preprocessor-s-memory-management.patchDownload+596-518
Hi,
On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
Here is a patch series that aims to clean up ecpg's preprocessor
code a little and fix the compile time problems we're seeing with
late-model clang [1]. I guess whether it's a cleanup is in the eye of
the beholder, but it definitely succeeds at fixing compile time: for
me, the time needed to compile preproc.o with clang 16 drops from
104 seconds to less than 1 second. It might be a little faster at
processing input too, though that wasn't the primary goal.
Nice! I'll look at this more later.
For now I just wanted to point one minor detail:
(If our code coverage tools worked on bison/flex stuff,
maybe this'd be less scary ... but they don't.)
For bison coverage seems to work, see e.g.:
https://coverage.postgresql.org/src/interfaces/ecpg/preproc/preproc.y.gcov.html#10638
I think the only reason it doesn't work for flex is that we have
/* LCOV_EXCL_START */
/* LCOV_EXCL_STOP */
around the scanner "body". Without that I get reasonable-looking, albeit not
very comforting, coverage for pgc.l as well.
|Lines |Functions|Branches
Filename |Rate Num|Rate Num|Rate Num
src/interfaces/ecpg/preproc/pgc.l |65.9% 748|87.5% 8| - 0
src/interfaces/ecpg/preproc/preproc.y |29.9% 4964|66.7% 15| - 0
This has been introduced by
commit 421167362242ce1fb46d6d720798787e7cd65aad
Author: Peter Eisentraut <peter_e@gmx.net>
Date: 2017-08-10 23:33:47 -0400
Exclude flex-generated code from coverage testing
Flex generates a lot of functions that are not actually used. In order
to avoid coverage figures being ruined by that, mark up the part of the
.l files where the generated code appears by lcov exclusion markers.
That way, lcov will typically only reported on coverage for the .l file,
which is under our control, but not for the .c file.
Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
but I don't think it's working as intended, as it's also preventing coverage
for the actual scanner definition.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
(If our code coverage tools worked on bison/flex stuff,
maybe this'd be less scary ... but they don't.)
For bison coverage seems to work, see e.g.:
Yeah, I'd just noticed that --- I had it in my head that we'd put
LCOV_EXCL_START/STOP into bison files too, but nope they are only
in flex files. That's good for this specific problem, because the
code I'm worried about is all in the bison file.
around the scanner "body". Without that I get reasonable-looking, albeit not
very comforting, coverage for pgc.l as well.
I was just looking locally at what I got by removing that, and sadly
I don't think I believe it: there are a lot of places where it claims
we hit lines we don't, and vice versa. That might be partially
blamable on old tools on my RHEL8 workstation, but it sure seems
that flex output confuses lcov to some extent.
regards, tom lane
On 2024-04-18 23:11:52 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
(If our code coverage tools worked on bison/flex stuff,
maybe this'd be less scary ... but they don't.)For bison coverage seems to work, see e.g.:
Yeah, I'd just noticed that --- I had it in my head that we'd put
LCOV_EXCL_START/STOP into bison files too, but nope they are only
in flex files. That's good for this specific problem, because the
code I'm worried about is all in the bison file.
At least locally the coverage seems to make sense too, both for the main
grammar and for ecpg's.
around the scanner "body". Without that I get reasonable-looking, albeit not
very comforting, coverage for pgc.l as well.I was just looking locally at what I got by removing that, and sadly
I don't think I believe it: there are a lot of places where it claims
we hit lines we don't, and vice versa. That might be partially
blamable on old tools on my RHEL8 workstation, but it sure seems
that flex output confuses lcov to some extent.
Hm. Here it mostly looks reasonable, except that at least things seem off by
1. And sure enough, if I look at pgc.l it has code like
case 2:
YY_RULE_SETUP
#line 465 "/home/andres/src/postgresql/src/interfaces/ecpg/preproc/pgc.l"
{
token_start = yytext;
state_before_str_start = YYSTATE;
However line 465 is actually the "token_start" line.
Further down this seems to get worse, by "<<EOF>>" it's off by 4 lines.
$ apt policy flex
flex:
Installed: 2.6.4-8.2+b2
Candidate: 2.6.4-8.2+b2
Version table:
*** 2.6.4-8.2+b2 500
500 http://mirrors.ocf.berkeley.edu/debian unstable/main amd64 Packages
100 /var/lib/dpkg/status
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2024-04-18 23:11:52 -0400, Tom Lane wrote:
I was just looking locally at what I got by removing that, and sadly
I don't think I believe it: there are a lot of places where it claims
we hit lines we don't, and vice versa. That might be partially
blamable on old tools on my RHEL8 workstation, but it sure seems
that flex output confuses lcov to some extent.
Hm. Here it mostly looks reasonable, except that at least things seem off by
1.
Yeah, now that you mention it what I'm seeing looks like the line
numbering might be off-by-one. Time for a bug report?
regards, tom lane
One other bit of randomness that I noticed: ecpg's parse.pl has
this undocumented bit of logic:
if ($a eq 'IDENT' && $prior eq '%nonassoc')
{
# add more tokens to the list
$str = $str . "\n%nonassoc CSTRING";
}
The net effect of that is that, where gram.y writes
%nonassoc UNBOUNDED NESTED /* ideally would have same precedence as IDENT */
%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
%left Op OPERATOR /* multi-character ops and user-defined operators */
preproc.c has
%nonassoc UNBOUNDED NESTED
%nonassoc IDENT
%nonassoc CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
%left Op OPERATOR
If you don't find that scary as heck, I suggest reading the very long
comment just in front of the cited lines of gram.y. The argument why
assigning these keywords a precedence at all is OK depends heavily
on it being the same precedence as IDENT, yet here's ECPG randomly
breaking that.
We seem to have avoided problems though, because if I fix things
by manually editing preproc.y to re-join the lines:
%nonassoc IDENT CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
the generated preproc.c doesn't change at all. Actually, I can
take CSTRING out of this list altogether and it still doesn't
change the results ... although looking at how CSTRING is used,
it looks safer to give it the same precedence as IDENT.
I think we should change parse.pl to give one or the other of these
results before something more serious breaks there.
regards, tom lane
The cfbot noticed that this patchset had a conflict with d35cd0619,
so here's a rebase. It's just a rebase of v1, no other changes.
regards, tom lane
Attachments:
v2-0001-Clean-up-indentation-and-whitespace-inconsistenci.patchtext/x-diff; charset=us-ascii; name*0=v2-0001-Clean-up-indentation-and-whitespace-inconsistenci.p; name*1=atchDownload+2608-2012
v2-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.patchtext/x-diff; charset=us-ascii; name*0=v2-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.p; name*1=atchDownload+120-64
v2-0003-Major-cleanup-simplification-and-documentation-of.patchtext/x-diff; charset=us-ascii; name*0=v2-0003-Major-cleanup-simplification-and-documentation-of.p; name*1=atchDownload+292-195
v2-0004-Re-implement-ecpg-preprocessor-s-string-managemen.patchtext/x-diff; charset=us-ascii; name*0=v2-0004-Re-implement-ecpg-preprocessor-s-string-managemen.p; name*1=atchDownload+752-1217
v2-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.patchtext/x-diff; charset=us-ascii; name*0=v2-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.p; name*1=atchDownload+107-90
v2-0006-Improve-ecpg-preprocessor-s-memory-management.patchtext/x-diff; charset=us-ascii; name=v2-0006-Improve-ecpg-preprocessor-s-memory-management.patchDownload+596-518
On Fri, Apr 19, 2024 at 10:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
One other bit of randomness that I noticed: ecpg's parse.pl has
this undocumented bit of logic:if ($a eq 'IDENT' && $prior eq '%nonassoc')
{# add more tokens to the list
$str = $str . "\n%nonassoc CSTRING";
}
preproc.c has
%nonassoc UNBOUNDED NESTED
%nonassoc IDENT
%nonassoc CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
%left Op OPERATORIf you don't find that scary as heck, I suggest reading the very long
comment just in front of the cited lines of gram.y. The argument why
assigning these keywords a precedence at all is OK depends heavily
on it being the same precedence as IDENT, yet here's ECPG randomly
breaking that.
Before 7f380c59f (Reduce size of backend scanner's tables), it was
even more spread out:
# add two more tokens to the list
$str = $str . "\n%nonassoc CSTRING\n%nonassoc UIDENT";
...giving:
%nonassoc UNBOUNDED
%nonassoc IDENT
%nonassoc CSTRING
%nonassoc UIDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS
PRECEDING FOLLOWING CUBE ROLLUP
We seem to have avoided problems though, because if I fix things
by manually editing preproc.y to re-join the lines:%nonassoc IDENT CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
the generated preproc.c doesn't change at all.
On a whim I tried rejoining on v12 and the .c doesn't change there, either.
Actually, I can
take CSTRING out of this list altogether and it still doesn't
change the results ... although looking at how CSTRING is used,
it looks safer to give it the same precedence as IDENT.
Doing that on v12 on top of rejoining results in a shift-reduce
conflict, so I imagine that's why it's there. Maybe it's outdated, but
this backs up your inclination that it's safer to keep.
On Fri, Jul 5, 2024 at 10:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
The cfbot noticed that this patchset had a conflict with d35cd0619,
so here's a rebase. It's just a rebase of v1, no other changes.
Hi Tom,
I started looking at the earlier cleanup patches.
0001 seems straightforward. Note: It doesn't apply cleanly anymore,
but does with 'patch'.
0002 LGTM, just a couple minor comments:
--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -1,7 +1,13 @@
#!/usr/bin/perl
# src/interfaces/ecpg/preproc/parse.pl
# parser generator for ecpg version 2
-# call with backend parser as stdin
+#
+# See README.parser for some explanation of what this does.Doesn't this patch set put us up to version 3? ;-) Looking in the
history, a very long time ago a separate "parse2.pl" was committed for
some reason, but that was reconciled some time later. This patch
doesn't need to get rid of that meaningless version number, but I find
it distracting.
+ # There may be multiple ECPG: lines and then multiple lines of code.
+ # Each block of code needs to be added to all prior ECPG records.This took me a while to parse at first. Some places in this script put
quotes around words-with-colons, and that seems good for readability.
0003:
Looks a heck of a lot better, but I didn't try to understand
everything in the script, either before or after.
+ # Emit the target part of the rule.
+ # Note: the leading space is just to match
+ # the old, rather weird output logic.
+ $tstr = ' ' . $non_term_id . ':';
+ add_to_buffer('rules', $tstr);Removing the leading space (or making it two spaces) has no effect on
the output -- does that get normalized elsewhere?
That's all I have for now.
John Naylor <johncnaylorls@gmail.com> writes:
I started looking at the earlier cleanup patches.
Thanks for looking!
0001 seems straightforward. Note: It doesn't apply cleanly anymore,
but does with 'patch'.
Odd, after rebasing it seems to have only line-number differences.
+ # Emit the target part of the rule. + # Note: the leading space is just to match + # the old, rather weird output logic. + $tstr = ' ' . $non_term_id . ':'; + add_to_buffer('rules', $tstr);
Removing the leading space (or making it two spaces) has no effect on
the output -- does that get normalized elsewhere?
It does affect horizontal space in the generated preproc.y file,
which'd have no effect on the derived preproc.c file. I tweaked
the commit message to clarify that.
I adopted your other suggestions, no need to rehash them.
Here's a rebased but otherwise identical patchset. I also added
an 0007 that removes check_rules.pl as threatened.
regards, tom lane
Attachments:
v3-0001-Clean-up-indentation-and-whitespace-inconsistenci.patchtext/x-diff; charset=us-ascii; name*0=v3-0001-Clean-up-indentation-and-whitespace-inconsistenci.p; name*1=atchDownload+2608-2012
v3-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.patchtext/x-diff; charset=us-ascii; name*0=v3-0002-Clean-up-documentation-of-parse.pl-and-add-more-i.p; name*1=atchDownload+123-66
v3-0003-Major-cleanup-simplification-and-documentation-of.patchtext/x-diff; charset=us-ascii; name*0=v3-0003-Major-cleanup-simplification-and-documentation-of.p; name*1=atchDownload+292-195
v3-0004-Re-implement-ecpg-preprocessor-s-string-managemen.patchtext/x-diff; charset=us-ascii; name*0=v3-0004-Re-implement-ecpg-preprocessor-s-string-managemen.p; name*1=atchDownload+752-1217
v3-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.patchtext/x-diff; charset=us-ascii; name*0=v3-0005-Move-some-functions-into-a-new-file-ecpg-preproc-.p; name*1=atchDownload+107-90
v3-0006-Improve-ecpg-preprocessor-s-memory-management.patchtext/x-diff; charset=us-ascii; name=v3-0006-Improve-ecpg-preprocessor-s-memory-management.patchDownload+596-518
v3-0007-Remove-ecpg-s-check_rules.pl.patchtext/x-diff; charset=us-ascii; name=v3-0007-Remove-ecpg-s-check_rules.pl.patchDownload+1-220
I wrote:
Here's a rebased but otherwise identical patchset. I also added
an 0007 that removes check_rules.pl as threatened.
I've done some more work on this and hope to post an updated patchset
tomorrow. Before that though, is there any objection to going ahead
with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
files)? It's pretty bulky yet of no intellectual interest, so I'd
like to stop carrying it forward.
regards, tom lane
On 15.08.24 02:43, Tom Lane wrote:
I wrote:
Here's a rebased but otherwise identical patchset. I also added
an 0007 that removes check_rules.pl as threatened.I've done some more work on this and hope to post an updated patchset
tomorrow. Before that though, is there any objection to going ahead
with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
files)? It's pretty bulky yet of no intellectual interest, so I'd
like to stop carrying it forward.
The indentation patch looks good to me and it would be good to get it
out of the way.
Peter Eisentraut <peter@eisentraut.org> writes:
On 15.08.24 02:43, Tom Lane wrote:
I've done some more work on this and hope to post an updated patchset
tomorrow. Before that though, is there any objection to going ahead
with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
files)? It's pretty bulky yet of no intellectual interest, so I'd
like to stop carrying it forward.
The indentation patch looks good to me and it would be good to get it
out of the way.
Thanks, done. Here's a revised patchset.
0001-0003 are substantially identical to the previous 0002-0004.
Likewise 0005 is basically the same as previous 0006,
and 0009 is identical to the previous 0007.
0004 differs from the previous 0005 in also moving the
cat_str/make_str functions into util.c, because I found that
at least the make_str functions could be useful in pgc.l.
The new stuff is in 0006-0008, and what it basically does is
clean up all remaining memory leakage in ecpg --- or at least,
all that valgrind can find while running ecpg's regression tests.
(I'm not fool enough to think that there might not be some in
unexercised code paths.) It's fairly straightforward attention
to detail in data structure management.
I discovered the need for more effort on memory leakage by
doing some simple performance testing (basically, running ecpg
on a big file made by pasting together lots of copies of some
of the regression test inputs). v3 was slower and consumed more
memory than HEAD :-(. HEAD does already leak quite a bit of
memory, but v3 was worse, mainly because the string tokens
returned by pgc.l weren't being reclaimed. I hadn't really
set out to drive the leakage to zero, but it turned out to not
be that hard, so I did it.
With those fixes, I see v4 running maybe 10% faster than HEAD,
rather than a similar amount slower. I'm content with that
result, and feel that this may now be commit-quality.
regards, tom lane
Attachments:
v4-0008-Clean-up-some-other-assorted-ecpg-memory-leaks.patchtext/x-diff; charset=us-ascii; name*0=v4-0008-Clean-up-some-other-assorted-ecpg-memory-leaks.patc; name*1=hDownload+86-54
v4-0009-Remove-ecpg-s-check_rules.pl.patchtext/x-diff; charset=us-ascii; name=v4-0009-Remove-ecpg-s-check_rules.pl.patchDownload+1-220
v4-0001-Clean-up-documentation-of-parse.pl-and-add-more-i.patchtext/x-diff; charset=us-ascii; name*0=v4-0001-Clean-up-documentation-of-parse.pl-and-add-more-i.p; name*1=atchDownload+123-66
v4-0002-Major-cleanup-simplification-and-documentation-of.patchtext/x-diff; charset=us-ascii; name*0=v4-0002-Major-cleanup-simplification-and-documentation-of.p; name*1=atchDownload+292-195
v4-0003-Re-implement-ecpg-preprocessor-s-string-managemen.patchtext/x-diff; charset=us-ascii; name*0=v4-0003-Re-implement-ecpg-preprocessor-s-string-managemen.p; name*1=atchDownload+752-1217
v4-0004-Move-some-functions-into-a-new-file-ecpg-preproc-.patchtext/x-diff; charset=us-ascii; name*0=v4-0004-Move-some-functions-into-a-new-file-ecpg-preproc-.p; name*1=atchDownload+195-154
v4-0005-Improve-ecpg-preprocessor-s-memory-management.patchtext/x-diff; charset=us-ascii; name=v4-0005-Improve-ecpg-preprocessor-s-memory-management.patchDownload+599-530
v4-0006-Fix-some-memory-leakage-of-data-type-related-stru.patchtext/x-diff; charset=us-ascii; name*0=v4-0006-Fix-some-memory-leakage-of-data-type-related-stru.p; name*1=atchDownload+29-13
v4-0007-Make-all-string-valued-tokens-returned-by-pgc.l-b.patchtext/x-diff; charset=us-ascii; name*0=v4-0007-Make-all-string-valued-tokens-returned-by-pgc.l-b.p; name*1=atchDownload+24-23
I wrote:
Thanks, done. Here's a revised patchset.
The cfbot points out that I should probably have marked progname
as "static" in 0008. I'm not going to repost the patchset just for
that, though.
regards, tom lane
I wrote:
The cfbot points out that I should probably have marked progname
as "static" in 0008. I'm not going to repost the patchset just for
that, though.
Rebase needed after f22e84df1, so here's an update that rebases
up to HEAD and adds the missing "static". No other changes.
(Anybody want to review this? I'm getting tired of rebasing it,
and we're missing out on the clang build time savings.)
regards, tom lane
Attachments:
v5-0001-Clean-up-documentation-of-parse.pl-and-add-more-i.patchtext/plain; charset=us-ascii; name*0=v5-0001-Clean-up-documentation-of-parse.pl-and-add-more-i.p; name*1=atchDownload+123-66
v5-0002-Major-cleanup-simplification-and-documentation-of.patchtext/x-diff; charset=us-ascii; name*0=v5-0002-Major-cleanup-simplification-and-documentation-of.p; name*1=atchDownload+292-195
v5-0003-Re-implement-ecpg-preprocessor-s-string-managemen.patchtext/x-diff; charset=us-ascii; name*0=v5-0003-Re-implement-ecpg-preprocessor-s-string-managemen.p; name*1=atchDownload+752-1217
v5-0004-Move-some-functions-into-a-new-file-ecpg-preproc-.patchtext/x-diff; charset=us-ascii; name*0=v5-0004-Move-some-functions-into-a-new-file-ecpg-preproc-.p; name*1=atchDownload+195-154
v5-0005-Improve-ecpg-preprocessor-s-memory-management.patchtext/x-diff; charset=us-ascii; name=v5-0005-Improve-ecpg-preprocessor-s-memory-management.patchDownload+599-530
v5-0006-Fix-some-memory-leakage-of-data-type-related-stru.patchtext/x-diff; charset=us-ascii; name*0=v5-0006-Fix-some-memory-leakage-of-data-type-related-stru.p; name*1=atchDownload+29-13
v5-0007-Make-all-string-valued-tokens-returned-by-pgc.l-b.patchtext/x-diff; charset=us-ascii; name*0=v5-0007-Make-all-string-valued-tokens-returned-by-pgc.l-b.p; name*1=atchDownload+24-23
v5-0008-Clean-up-some-other-assorted-ecpg-memory-leaks.patchtext/x-diff; charset=us-ascii; name*0=v5-0008-Clean-up-some-other-assorted-ecpg-memory-leaks.patc; name*1=hDownload+86-54
v5-0009-Remove-ecpg-s-check_rules.pl.patchtext/x-diff; charset=us-ascii; name=v5-0009-Remove-ecpg-s-check_rules.pl.patchDownload+1-220
On Saturday, October 5, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Rebase needed after f22e84df1, so here's an update that rebases
up to HEAD and adds the missing "static". No other changes.(Anybody want to review this? I'm getting tired of rebasing it,
and we're missing out on the clang build time savings.)
Sorry for the delay, I'll respond in a couple days.
[v5]
0001 - LGTM, maybe can be squashed with 0009?
0002 - I went through this again and don't see anything that should
raise eyebrows.
+ # HACK: insert our own %nonassoc line after IDENT.
+ # XXX: this seems pretty wrong, IDENT is not last on its line!We can come back to this afterwards, as mentioned elsewhere in the thread.
0003
Clang is what motivated this, but gcc also shows a speedup -- from
5.9s to 1.6s on this old machine, which is great. The giant switch
statement in preproc.c has about 1/10 the labels as before.
+In the original implementation of ecpg, the strings constructed
+by grammar rules were returned as the Bison result of each rule.
+This led to a large number of effectively-identical rule actions,
+which caused compilation-time problems with some versions of clang.
+Now, rules that need to return a string are declared as having"Original" is going to be a mystery in a few years -- I'd describe
this in terms of "as of PG18" or some such.
+ * is producing uniformly-cased output of keywords. (That's mostly
+ * cosmetic, but there are places in ecpglib that expect to receive
+ * downcased keywords, plus it keeps us regression-test-compatible
+ * with the old implementation of ecpg.)Ditto with "old".
+ /* List a token here if pgc.l assigns to base_yylval.str for it */
Does pgc.l need to have a similar comment?
0004 seems like a sensible reorg.
0005 - I wondered if the change of YYLTYPE to "const char *" can be
done in a separate commit to make the other changes more legible, but
that might not be worth the effort.
+ * "Local" (or "location"?) memory management support
"Local" seems to fit well enough. Tying the arena to the statement
level seems sound.
I haven't looked closely at 0006 through 0009. One possible concern is
that the regression tests might not cover very well, but if you can
get valgrind silent for memory leaks for what they do cover, that's
certainly a good step.
John Naylor <johncnaylorls@gmail.com> writes:
[v5]
Thanks for reviewing! I pushed 0001-0005 and 0009, adopting
your suggestions except for
+ /* List a token here if pgc.l assigns to base_yylval.str for it */
Does pgc.l need to have a similar comment?
That's not a bad suggestion, but I couldn't see any very useful place
to put such a comment.
I haven't looked closely at 0006 through 0009. One possible concern is
that the regression tests might not cover very well, but if you can
get valgrind silent for memory leaks for what they do cover, that's
certainly a good step.
Attached are rebased and renumbered 0006-0008, mostly to keep the
cfbot happy. We could actually stop here, if we were feeling lazy,
but now that I've done the work I'm inclined to push forward with
the rest.
The rest is just memory leak removal, and I suspect that nobody really
cares that much about small leakage in the preprocessor: you'd have to
be running some darn big files through it to notice. FTR, here are
the total leaks reported by valgrind for running the ecpg regression
tests, using code like
$ grep lost: *log | tr -d ',' | awk '{sum += $5}
END {print sum}'
Before these patches: 25743
after 0003: 59049363
after 0005: 141556 (this is master now)
after 0006(0001): 132633
after 0007(0002): 9087
after 0008(0003): 0
So clearly, 0003 by itself wasn't good enough, but arguably no
real users will notice the extra inefficiency as of HEAD.
Still, I'd kind of like to get 0007 (now 0002) in there, and
I believe 0006 (0001) is a necessary prerequisite to that.
regards, tom lane
Attachments:
v6-0001-ecpg-fix-some-memory-leakage-of-data-type-related.patchtext/x-diff; charset=us-ascii; name*0=v6-0001-ecpg-fix-some-memory-leakage-of-data-type-related.p; name*1=atchDownload+29-13
v6-0002-ecpg-put-all-string-valued-tokens-returned-by-pgc.patchtext/x-diff; charset=us-ascii; name*0=v6-0002-ecpg-put-all-string-valued-tokens-returned-by-pgc.p; name*1=atchDownload+24-23
v6-0003-ecpg-clean-up-some-other-assorted-memory-leaks.patchtext/x-diff; charset=us-ascii; name*0=v6-0003-ecpg-clean-up-some-other-assorted-memory-leaks.patc; name*1=hDownload+86-54
Hello Tom,
14.10.2024 21:25, Tom Lane wrote:
Attached are rebased and renumbered 0006-0008, mostly to keep the
cfbot happy. We could actually stop here, if we were feeling lazy,
but now that I've done the work I'm inclined to push forward with
the rest.The rest is just memory leak removal, and I suspect that nobody really
cares that much about small leakage in the preprocessor: you'd have to
be running some darn big files through it to notice. FTR, here are
the total leaks reported by valgrind for running the ecpg regression
tests, using code like
Maybe you would like to fix in passing several (not new) defects, I've
found while playing with ecpg under Valgrind:
echo "
EXEC SQL DECLARE cur1 CURSOR FOR stmt1;
" > t.pgc
valgrind .../preproc/ecpg ... t.pgc
==831888== Conditional jump or move depends on uninitialised value(s)
==831888== at 0x10C7B0: main (ecpg.c:490)
==831888==
char_array.pgc:2: WARNING: cursor "cur1" has been declared but not opened
Another case:
EXEC SQL DECLARE cur_1 CURSOR FOR stmt_1;
EXEC SQL FETCH cur_1 INTO :f1[[i];
==1335775==
==1335775== Conditional jump or move depends on uninitialised value(s)
==1335775== at 0x121294: find_variable (variable.c:211)
==1335775== by 0x11D661: base_yyparse (preproc.y:9749)
==1335775== by 0x10C78F: main (ecpg.c:483)
==1335775==
==1335775== Conditional jump or move depends on uninitialised value(s)
==1335775== at 0x121299: find_variable (variable.c:211)
==1335775== by 0x11D661: base_yyparse (preproc.y:9749)
==1335775== by 0x10C78F: main (ecpg.c:483)
==1335775==
==1335775== Invalid read of size 1
==1335775== at 0x12128B: find_variable (variable.c:211)
==1335775== by 0x11D661: base_yyparse (preproc.y:9749)
==1335775== by 0x10C78F: main (ecpg.c:483)
==1335775== Address 0x4e3bc80 is 0 bytes after a block of size 8,208 alloc'd
==1335775== at 0x4848899: malloc (vg_replace_malloc.c:381)
==1335775== by 0x120585: mm_alloc (util.c:87)
==1335775== by 0x12065A: loc_alloc (util.c:151)
==1335775== by 0x120701: loc_strdup (util.c:172)
==1335775== by 0x10D9EC: base_yylex_location (parser.c:261)
==1335775== by 0x10D4A1: filtered_base_yylex (parser.c:75)
==1335775== by 0x114CA4: base_yyparse (preproc.c:39316)
==1335775== by 0x10C78F: main (ecpg.c:483)
==1335775==
declare.pgc:2: ERROR: variable "f1" is not declared
One more case:
EXEC SQL BEGIN DECLARE SECTION;
int i;
EXEC SQL END DECLARE SECTION;
EXEC SQL DECLARE C CURSOR FOR SELECT 1;
{
EXEC SQL FETCH 1 IN C INTO :i;
}
EXEC SQL MOVE BACKWARD 1 IN C;
==961441== Invalid read of size 1
==961441== at 0x484FBD7: strcmp (vg_replace_strmem.c:924)
==961441== by 0x11442F: add_additional_variables (preproc.y:470)
==961441== by 0x117DEF: base_yyparse (preproc.y:3548)
==961441== by 0x10C78F: main (ecpg.c:483)
==961441== Address 0x0 is not stack'd, malloc'd or (recently) free'd
Best regards,
Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
Maybe you would like to fix in passing several (not new) defects, I've
found while playing with ecpg under Valgrind:
Done. After evaluation I concluded that none of these were worth the
trouble to back-patch, but by all means let's fix such things in HEAD.
regards, tom lane