Horton

Thread

Commits

  1. Fix assorted bugs in ecpg's macro mechanism.

  1. Bugs in ecpg's macro mechanism

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-15T21:48:32Z

    I started looking into the ideas discussed at [1] about reimplementing
ecpg's string handling.  Before I could make any progress I needed
to understand the existing input code, part of which is the macro
expansion mechanism ... and the more I looked at that the more bugs
I found, not to mention that it uses misleading field names and is
next door to uncommented.  I found two ways to crash ecpg outright
and several more cases in which it'd produce surprising behavior.
As an example,

$ cd .../src/interfaces/ecpg/test/preproc/
$ ../../preproc/ecpg --regression -I./../../include -I. -DNAMELEN=99 -o define.c define.pgc
munmap_chunk(): invalid pointer
Aborted

Attached is a patch that cleans all that up and attempts to add a
little documentation about how things work.  One thing it's missing
is any test of the behavior when command-line macro definitions are
carried from one file to the next one.  To test that, we'd need to
compile more than one ecpg input file at a time.  I can see how
to kluge the Makefiles to make that happen, basically this'd do:

 define.c: define.pgc $(ECPG_TEST_DEPENDENCIES)
-	$(ECPG) -DCMDLINESYM=123 -o $@ $<
+	$(ECPG) -DCMDLINESYM=123 -o $@ $< $<

But I have no idea about making it work in meson.  Any suggestions?

			regards, tom lane

[1] https://www.postgresql.org/message-id/3897526.1712710536%40sss.pgh.pa.us

  2. Re: Bugs in ecpg's macro mechanism

    Andres Freund <andres@anarazel.de> — 2024-04-15T23:10:44Z

    Hi,

On 2024-04-15 17:48:32 -0400, Tom Lane wrote:
> I started looking into the ideas discussed at [1] about reimplementing
> ecpg's string handling.  Before I could make any progress I needed
> to understand the existing input code, part of which is the macro
> expansion mechanism ... and the more I looked at that the more bugs
> I found, not to mention that it uses misleading field names and is
> next door to uncommented.

As part of the discussion leading to [1] I had looked at parse.pl and found it
fairly impressively obfuscated and devoid of helpful comments.


> I found two ways to crash ecpg outright and several more cases in which it'd
> produce surprising behavior.

:/


> One thing it's missing is any test of the behavior when command-line macro
> definitions are carried from one file to the next one.  To test that, we'd
> need to compile more than one ecpg input file at a time.  I can see how to
> kluge the Makefiles to make that happen, basically this'd do:
> 
>  define.c: define.pgc $(ECPG_TEST_DEPENDENCIES)
> -	$(ECPG) -DCMDLINESYM=123 -o $@ $<
> +	$(ECPG) -DCMDLINESYM=123 -o $@ $< $<
> 
> But I have no idea about making it work in meson.  Any suggestions?

So you just want to compile define.c twice? The below should suffice:

diff --git i/src/interfaces/ecpg/test/sql/meson.build w/src/interfaces/ecpg/test/sql/meson.build
index e04684065b0..202dc69c6ea 100644
--- i/src/interfaces/ecpg/test/sql/meson.build
+++ w/src/interfaces/ecpg/test/sql/meson.build
@@ -31,7 +31,7 @@ pgc_files = [
 ]
 
 pgc_extra_flags = {
-  'define': ['-DCMDLINESYM=123'],
+  'define': ['-DCMDLINESYM=123', files('define.pgc')],
   'oldexec': ['-r', 'questionmarks'],
 }
 

I assume that was just an test hack, because it leads to the build failing
because of main being duplicated. But it'd work the same with another, "non
overlapping", file.

Greetings,

Andres Freund

  3. Re: Bugs in ecpg's macro mechanism

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-16T00:47:16Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-04-15 17:48:32 -0400, Tom Lane wrote:
>> But I have no idea about making it work in meson.  Any suggestions?

> So you just want to compile define.c twice? The below should suffice:

> -  'define': ['-DCMDLINESYM=123'],
> +  'define': ['-DCMDLINESYM=123', files('define.pgc')],

Ah, thanks.  I guess this depends on getopt_long reordering arguments
(since the "-o outfile" bit will come later).  That is safe enough
in HEAD since 411b72034, but it might fail on weird platforms in v16.
How much do we care about that?  (We can avoid that hazard in the
makefile build easily enough.)

> I assume that was just an test hack, because it leads to the build failing
> because of main being duplicated. But it'd work the same with another, "non
> overlapping", file.

Yeah, I hadn't actually worked through what to do in detail.
Here's a v2 that adds that testing.  I also added some more
user-facing doco, and fixed a small memory leak that I noted
from valgrind testing.  (It's hardly the only one in ecpg,
but it was easy to fix as part of this patch.)

			regards, tom lane

  4. Re: Bugs in ecpg's macro mechanism

    Andres Freund <andres@anarazel.de> — 2024-04-16T01:05:44Z

    Hi,

On 2024-04-15 20:47:16 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-04-15 17:48:32 -0400, Tom Lane wrote:
> >> But I have no idea about making it work in meson.  Any suggestions?
> 
> > So you just want to compile define.c twice? The below should suffice:
> 
> > -  'define': ['-DCMDLINESYM=123'],
> > +  'define': ['-DCMDLINESYM=123', files('define.pgc')],
> 
> Ah, thanks.  I guess this depends on getopt_long reordering arguments
> (since the "-o outfile" bit will come later).  That is safe enough
> in HEAD since 411b72034, but it might fail on weird platforms in v16.
> How much do we care about that?  (We can avoid that hazard in the
> makefile build easily enough.)

Oh, I didn't even think of that. If we do care, we can just move the -o to
earlier. Or just officially add it as another input, that'd just be a bit of
notational overhead.

As moving the arguments around would just be the following, I see no reason to
just do so.

diff --git i/src/interfaces/ecpg/test/meson.build w/src/interfaces/ecpg/test/meson.build
index c1e508ccc82..d7c0e9de7d6 100644
--- i/src/interfaces/ecpg/test/meson.build
+++ w/src/interfaces/ecpg/test/meson.build
@@ -45,9 +45,10 @@ ecpg_preproc_test_command_start = [
   '--regression',
   '-I@CURRENT_SOURCE_DIR@',
   '-I@SOURCE_ROOT@' + '/src/interfaces/ecpg/include/',
+  '-o', '@OUTPUT@',
 ]
 ecpg_preproc_test_command_end = [
-  '-o', '@OUTPUT@', '@INPUT@'
+  '@INPUT@',
 ]
 
 ecpg_test_dependencies = []


Greetings,

Andres Freund

  5. Re: Bugs in ecpg's macro mechanism

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-16T01:33:34Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-04-15 20:47:16 -0400, Tom Lane wrote:
>> Ah, thanks.  I guess this depends on getopt_long reordering arguments
>> (since the "-o outfile" bit will come later).  That is safe enough
>> in HEAD since 411b72034, but it might fail on weird platforms in v16.
>> How much do we care about that?  (We can avoid that hazard in the
>> makefile build easily enough.)

> As moving the arguments around would just be the following, I see no reason to
> just do so.

Fair enough.  I'm inclined to include that change only in v16, though.

			regards, tom lane