Thread: Implicit make rules break test examples

Hi,

In src/test/example, the implicit make rules produce errors:

make -C ../../../src/backend generated-headers
make[1]: Entering directory '/home/ddong/postgresql/bld/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory '/home/ddong/postgresql/bld/src/backend/catalog'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/ddong/postgresql/bld/src/backend/catalog'
make -C utils distprep generated-header-symlinks
make[2]: Entering directory '/home/ddong/postgresql/bld/src/backend/utils'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/ddong/postgresql/bld/src/backend/utils'
make[1]: Leaving directory '/home/ddong/postgresql/bld/src/backend'
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -Wno-format-truncation -O2
-I/home/ddong/postgresql/bld/../src/interfaces/libpq
-I../../../src/include -I/home/ddong/postgresql/bld/../src/include
-D_GNU_SOURCE   -c -o testlibpq.o
/home/ddong/postgresql/bld/../src/test/examples/testlibpq.c
gcc -L../../../src/port -L../../../src/common -L../../../src/common
-lpgcommon -L../../../src/port -lpgport
-L../../../src/interfaces/libpq -lpq   -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags  testlibpq.o   -o
testlibpq
testlibpq.o: In function `exit_nicely':
testlibpq.c:(.text.unlikely+0x5): undefined reference to `PQfinish'
testlibpq.o: In function `main':
testlibpq.c:(.text.startup+0x22): undefined reference to `PQconnectdb'
testlibpq.c:(.text.startup+0x2d): undefined reference to `PQstatus'
testlibpq.c:(.text.startup+0x44): undefined reference to `PQexec’
…

I think the -lpq flag does not have any effects in the middle of the
arguments. It works if move the flag to the end:

gcc -L../../../src/port -L../../../src/common -L../../../src/common
-lpgcommon -L../../../src/port -lpgport
-L../../../src/interfaces/libpq -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags  testlibpq.o   -o
testlibpq -lpq

So I added an explicit rule to rearrange the flags:

gcc testlibpq.o -o testlibpq -L../../../src/port -L../../../src/common
-L../../../src/common -lpgcommon -L../../../src/port -lpgport
-L../../../src/interfaces/libpq -lpq   -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags

Then the make command works as expected. This is my first time writing
a patch. Please let me know what you think!

Thank you,
Happy new year!
Donald Dong

On Mon, Dec 31, 2018 at 11:24 PM Donald Dong <xdong@csumb.edu> wrote:
>
> Hi,
>
> In src/test/example, the implicit make rules produce errors:
>
> make -C ../../../src/backend generated-headers
> make[1]: Entering directory '/home/ddong/postgresql/bld/src/backend'
> make -C catalog distprep generated-header-symlinks
> make[2]: Entering directory '/home/ddong/postgresql/bld/src/backend/catalog'
> make[2]: Nothing to be done for 'distprep'.
> make[2]: Nothing to be done for 'generated-header-symlinks'.
> make[2]: Leaving directory '/home/ddong/postgresql/bld/src/backend/catalog'
> make -C utils distprep generated-header-symlinks
> make[2]: Entering directory '/home/ddong/postgresql/bld/src/backend/utils'
> make[2]: Nothing to be done for 'distprep'.
> make[2]: Nothing to be done for 'generated-header-symlinks'.
> make[2]: Leaving directory '/home/ddong/postgresql/bld/src/backend/utils'
> make[1]: Leaving directory '/home/ddong/postgresql/bld/src/backend'
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -Wno-format-truncation -O2
> -I/home/ddong/postgresql/bld/../src/interfaces/libpq
> -I../../../src/include -I/home/ddong/postgresql/bld/../src/include
> -D_GNU_SOURCE   -c -o testlibpq.o
> /home/ddong/postgresql/bld/../src/test/examples/testlibpq.c
> gcc -L../../../src/port -L../../../src/common -L../../../src/common
> -lpgcommon -L../../../src/port -lpgport
> -L../../../src/interfaces/libpq -lpq   -Wl,--as-needed
> -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags  testlibpq.o   -o
> testlibpq
> testlibpq.o: In function `exit_nicely':
> testlibpq.c:(.text.unlikely+0x5): undefined reference to `PQfinish'
> testlibpq.o: In function `main':
> testlibpq.c:(.text.startup+0x22): undefined reference to `PQconnectdb'
> testlibpq.c:(.text.startup+0x2d): undefined reference to `PQstatus'
> testlibpq.c:(.text.startup+0x44): undefined reference to `PQexec’
> …
>
> I think the -lpq flag does not have any effects in the middle of the
> arguments. It works if move the flag to the end:
>
> gcc -L../../../src/port -L../../../src/common -L../../../src/common
> -lpgcommon -L../../../src/port -lpgport
> -L../../../src/interfaces/libpq -Wl,--as-needed
> -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags  testlibpq.o   -o
> testlibpq -lpq
>
> So I added an explicit rule to rearrange the flags:
>
> gcc testlibpq.o -o testlibpq -L../../../src/port -L../../../src/common
> -L../../../src/common -lpgcommon -L../../../src/port -lpgport
> -L../../../src/interfaces/libpq -lpq   -Wl,--as-needed
> -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags
>
> Then the make command works as expected. This is my first time writing
> a patch. Please let me know what you think!
>
> Thank you,
> Happy new year!
> Donald Dong



--
Donald Dong

Donald Dong <xdong@csumb.edu> writes:
> In src/test/example, the implicit make rules produce errors:

Hm.  "make" in src/test/examples works fine for me.

The only way I can account for the results you're showing is if your
linker is preferring libpq.a to libpq.so, so that reading the library
before the *.o files causes none of it to get pulled in.  But that
isn't the default behavior on any modern platform AFAIK, and certainly
isn't considered good practice these days.  Moreover, if that's what's
happening, I don't see how you would have managed to build PG at all,
because there are a lot of other places where our Makefiles write
$(LDFLAGS) before the *.o files they're trying to link.  Maybe we
shouldn't have done it like that, but it's been working for everybody
else.

What platform are you on exactly, and what toolchain (gcc and ld
versions) are you using?

            regards, tom lane

Donald Dong <xdong@csumb.edu> writes:
> Thank you for the explanation! That makes sense. It is strange that it does
> not work for me.

Yeah, I still can't account for the difference in behavior between your
platform and mine (I tried several different ones here, and they all
manage to build src/test/examples).  However, I'm now convinced that
we do have an issue, because I found another place that does fail on my
platforms: src/interfaces/libpq/test gives failures like

uri-regress.o: In function `main':
uri-regress.c:58: undefined reference to `pg_printf'

the reason being that the link command lists -lpgport before
uri-regress.o, and since we only make the .a flavor of libpgport, it's
definitely going to be order-sensitive.  (This has probably been busted
since we changed over to using snprintf.c everywhere, but nobody noticed
because this test isn't normally run.)

The reason we haven't noticed this already seems to be that in all the
places where it matters, we have explicit link rules that order things
like this:

    $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

However, the places that are having problems are trying to rely on
gmake's implicit rule, which according to their manual is

  Linking a single object file
     `N' is made automatically from `N.o' by running the linker
     (usually called `ld') via the C compiler.  The precise command
     used is `$(CC) $(LDFLAGS) N.o $(LOADLIBES) $(LDLIBS)'.

So really the problem here is that we're doing the wrong thing by
injecting -l switches into LDFLAGS; it would be more standard to
put them into LDLIBS (or maybe LOADLIBES, though I think that's
not commonly used).

I hesitate to try to change that though.  The places that are messing with
that are injecting both -L and -l switches, and we want to keep putting
the -L switches into LDFLAGS because of the strong likelihood that the
initial (autoconf-derived) value of LDFLAGS contains -L switches; our
switches pointing at within-tree directories need to come first.
So the options seem to be:

1. Redefine our makefile conventions as being that internal -L switches
go into LDFLAGS_INTERNAL but internal -l switches go into LDLIBS_INTERNAL,
and we use the same recursive-expansion dance for LDLIBS[_INTERNAL] as for
LDFLAGS[_INTERNAL], and we have to start mentioning LDLIBS in our explicit
link rules.  This would be a pretty invasive patch, I'm afraid, and would
almost certainly break some third-party extensions' Makefiles.  It'd be
the cleanest solution in a green field, I think, but our Makefiles are
hardly a green field.

2. Be sure to override the gmake implicit link rule with an explicit
link rule everywhere --- basically like your patch, but touching more
places.  This seems like the least risky alternative in the short
run, but we'd be highly likely to reintroduce such problems in future.

3. Replace the default implicit link rule with one of our own.
Conceivably this also breaks some extensions' Makefiles, though
I think that's less likely.

I observe that ecpg's Makefile.regress is already doing #3:

%: %.o
    $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@

so what we'd be talking about is moving that to some more global spot,
probably Makefile.global.  (I wonder why the target is not specified
as $@$(X) here?)

I notice that the platform-specific makefiles in src/makefiles
are generally also getting it wrong in their implicit rules for
building shlibs, eg in Makefile.linux:

# Rule for building a shared library from a single .o file
%.so: %.o
    $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@ $<

Per this discussion, that needs to be more like

    $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@

in case a reference to libpgport or libpgcommon has been inserted
into LDFLAGS.  I'm a bit surprised that that hasn't bitten us
already.

            regards, tom lane

> I observe that ecpg's Makefile.regress is already doing #3:
> %: %.o
>         $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
> so what we'd be talking about is moving that to some more global spot,
> probably Makefile.global.  (I wonder why the target is not specified
> as $@$(X) here?)


Thank you for pointing that out!
I think #3 is a better choice since it's less invasive and would
potentially avoid similar problems in the future. I think may worth
the risks of breaking some extensions. I moved the rule to the
Makefile.global and added  $(X) in case it's set.

I also updated the order in Makefile.linux in the same patch since
they have the same cause. I'm not sure if changes are necessary for
other platforms, and I am not able to test it, unfortunately.

I've built it again on Ubuntu and tested src/test/examples and
src/interfaces/libpq/test. There are no errors.

Thank you again for the awesome explanation,
Donald Dong

On Tue, Jan 1, 2019 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Donald Dong <xdong@csumb.edu> writes:
> > Thank you for the explanation! That makes sense. It is strange that it does
> > not work for me.
>
> Yeah, I still can't account for the difference in behavior between your
> platform and mine (I tried several different ones here, and they all
> manage to build src/test/examples).  However, I'm now convinced that
> we do have an issue, because I found another place that does fail on my
> platforms: src/interfaces/libpq/test gives failures like
>
> uri-regress.o: In function `main':
> uri-regress.c:58: undefined reference to `pg_printf'
>
> the reason being that the link command lists -lpgport before
> uri-regress.o, and since we only make the .a flavor of libpgport, it's
> definitely going to be order-sensitive.  (This has probably been busted
> since we changed over to using snprintf.c everywhere, but nobody noticed
> because this test isn't normally run.)
>
> The reason we haven't noticed this already seems to be that in all the
> places where it matters, we have explicit link rules that order things
> like this:
>
>         $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
>
> However, the places that are having problems are trying to rely on
> gmake's implicit rule, which according to their manual is
>
>   Linking a single object file
>      `N' is made automatically from `N.o' by running the linker
>      (usually called `ld') via the C compiler.  The precise command
>      used is `$(CC) $(LDFLAGS) N.o $(LOADLIBES) $(LDLIBS)'.
>
> So really the problem here is that we're doing the wrong thing by
> injecting -l switches into LDFLAGS; it would be more standard to
> put them into LDLIBS (or maybe LOADLIBES, though I think that's
> not commonly used).
>
> I hesitate to try to change that though.  The places that are messing with
> that are injecting both -L and -l switches, and we want to keep putting
> the -L switches into LDFLAGS because of the strong likelihood that the
> initial (autoconf-derived) value of LDFLAGS contains -L switches; our
> switches pointing at within-tree directories need to come first.
> So the options seem to be:
>
> 1. Redefine our makefile conventions as being that internal -L switches
> go into LDFLAGS_INTERNAL but internal -l switches go into LDLIBS_INTERNAL,
> and we use the same recursive-expansion dance for LDLIBS[_INTERNAL] as for
> LDFLAGS[_INTERNAL], and we have to start mentioning LDLIBS in our explicit
> link rules.  This would be a pretty invasive patch, I'm afraid, and would
> almost certainly break some third-party extensions' Makefiles.  It'd be
> the cleanest solution in a green field, I think, but our Makefiles are
> hardly a green field.
>
> 2. Be sure to override the gmake implicit link rule with an explicit
> link rule everywhere --- basically like your patch, but touching more
> places.  This seems like the least risky alternative in the short
> run, but we'd be highly likely to reintroduce such problems in future.
>
> 3. Replace the default implicit link rule with one of our own.
> Conceivably this also breaks some extensions' Makefiles, though
> I think that's less likely.
>
> I observe that ecpg's Makefile.regress is already doing #3:
>
> %: %.o
>         $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
>
> so what we'd be talking about is moving that to some more global spot,
> probably Makefile.global.  (I wonder why the target is not specified
> as $@$(X) here?)
>
> I notice that the platform-specific makefiles in src/makefiles
> are generally also getting it wrong in their implicit rules for
> building shlibs, eg in Makefile.linux:
>
> # Rule for building a shared library from a single .o file
> %.so: %.o
>         $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@ $<
>
> Per this discussion, that needs to be more like
>
>         $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_SL) -shared -o $@
>
> in case a reference to libpgport or libpgcommon has been inserted
> into LDFLAGS.  I'm a bit surprised that that hasn't bitten us
> already.
>
>                         regards, tom lane



-- 
Donald Dong

Donald Dong <xdong@csumb.edu> writes:
> I think #3 is a better choice since it's less invasive and would
> potentially avoid similar problems in the future. I think may worth
> the risks of breaking some extensions. I moved the rule to the
> Makefile.global and added  $(X) in case it's set.

Yeah, I think #3 is the best choice too.

I'm not quite sure about the $(X) addition --- it makes the output
file not agree with what gmake thinks the target is.  However, I
observe other places doing the same thing, so let's try that and
see what the buildfarm thinks.

> I also updated the order in Makefile.linux in the same patch since
> they have the same cause. I'm not sure if changes are necessary for
> other platforms, and I am not able to test it, unfortunately.

That's what we have a buildfarm for.  Pushed, we'll soon find out.

            regards, tom lane