Re: Implicit make rules break test examples

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Donald Dong <xdong(at)csumb(dot)edu>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Implicit make rules break test examples
Date: 2019-01-01 19:54:50
Message-ID: 10415.1546372490@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Donald Dong <xdong(at)csumb(dot)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 $(at)$(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 $(at)$(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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nikolay Shaplov 2019-01-01 20:21:45 Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
Previous Message Pavel Stehule 2019-01-01 19:18:38 Re: explain plans with information about (modified) gucs