src/Makefile: do not override LIBS and CFLAGS for prerequisites

[trofi]

Without the change make --shuffle build occasionally fails as:

$ gcc -Og  -g3 -Wall -Wextra  -Werror  -std=gnu11 -funsigned-char -fvisibility=hidden -specs=/build/source/src/include/gcc.specs -fno-merge-constants  -L.   -Wl,--build-id -Wl,--no-allow-shlib-undefined -Wl,--no-undefined-version -Wl,-z,now -Wl,-z,muldefs -Wl,-z,relro -Wl,--fatal-warnings     -DLIBEFIVAR_VERSION=38 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -I/build/source/src/include/  -shared -Wl,-soname,libefisec.so.1 -Wl,--version-script=libefisec.map  -o libefisec.so sec.o secdb.o esl-iter.o util.o -lefivar -lefisec -ldl
/nix/store/zwqkxpi1iz66mix0kirdaq2ps6a9g9cg-binutils-2.41/bin/ld: cannot find -lefivar: No such file or directory
collect2: error: ld returned 1 exit status
make[1]: *** [/build/source/src/include/rules.mk:38: libefisec.so] Error 1 shuffle=721268944

Before the change 2-3 rebuild attemts were enough to trigger build failure.

After the change evivar survived 100 rebuilds.

Artem Klimov noticed that it happens because LIBS gets overridden
(or not overridden) based on the traversal order make takes to build
the prerequisites.

If the order is:

all -> libefivar.so

then LIBS is taken from libefivar.so target, which is

libefivar.so : LIBS=dl

There are no prerequisites and all is fine.

But if the build order starts from efisecdb, then:

efisecdb -> libefisec.so

then LIBS is taken from efisecdb, this is:

efisecdb : $(EFISECDB_OBJECTS) | libefisec.so
efisecdb : private LIBS=efivar efisec dl

When these LIBS are propagated to libefisec.so we get a linking
failure.

Thus the fix is to mark all LIBS instances as private to make sure
LIBS never gets leaked into prerequisites' LIBS use. And while at it
do the same for local MAP and local CFLAGS for consistency.

[JakMobius]

libefisec and libefivar are actually independent from each other. Take a look at the regular gcc invocation when building libefisec.so:

$ make -j 16 | grep -- "-o libefisec.so"
	-o libefisec.so sec.o secdb.o esl-iter.o util.o  

Notice that -lpthread and -lefivar options are not present here, although the build ended up being successful. It looks like that the problem is with LIBS variable, which should be empty when building libefisec.so.

The only place where LIBS is assigned -lpthread -lefivar is the thread-test target:

$ grep "LIBS=" . -r
./src/Makefile:makeguids : LIBS=dl
./src/Makefile:libefivar.so : LIBS=dl
./src/Makefile:efivar : LIBS=efivar dl
./src/Makefile:efivar-static : LIBS=dl
./src/Makefile:libefiboot.so : LIBS=efivar
./src/Makefile:efisecdb : LIBS=efivar efisec dl
./src/Makefile:efisecdb-static : LIBS=dl
./src/Makefile:thread-test : LIBS=pthread efivar
./src/test/Makefile:LIBS=efivar
./src/include/defaults.mk:LDLIBS=$(foreach lib,$(LIBS),-l$(lib)) $(call pkg-config-ldlibs)

So it seems like your make --shuffle happened to build thread-test just before libefisec.so, setting the LIBS to -lpthread -lefivar. The libefisec.so target, in its turn, did not set LIBS to something else, and just used it right away:

src/Makefile:115:

#...
libefisec.so : $(LIBEFISEC_OBJECTS)
libefisec.so : | libefisec.map
libefisec.so : MAP=libefisec.map
#...

By the way, all other .so targets set LIBS to their own value. It can be seen from the grep output above.libefisec.so is the only library that does not have any such overrides.

So while your patch fixes the occasional build failure, the issue is still there and the libefisec.so library may get linked against libpthread and libefivar, which is not expected to happen.

I suggest adding private to the variable assignments on thread-test target:

diff --git a/src/Makefile b/src/Makefile
index 0e423c4..bce080e 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -126,7 +126,7 @@ efisecdb-static : LIBS=dl
 
 thread-test : libefivar.so
 thread-test : CFLAGS=$(HOST_CFLAGS) -I$(TOPDIR)/src/include/efivar
-thread-test : LIBS=pthread efivar
+thread-test : private LIBS=pthread efivar
 
 deps : $(ALL_SOURCES)
        @$(MAKE) -f $(SRCDIR)/include/deps.mk deps SOURCES="$(ALL_SOURCES)"

[JakMobius]

A similar problem can be seen with CFLAGS variable, as it's being set to $(HOST_CFLAGS) -I$(TOPDIR)/src/include/efivar in the thread-test target. This value can also leak in different targets. It wouldn't have caused significant problems since the only difference is -I$(TOPDIR)/src/include/efivar , but it's likely not intended.

In this case, the private modifier would prevent the flags from being inherited in subtargets, leading to a #include resolution failure when building thread-test.o. However, thread-test.o is the only target that requires modified CFLAGS. So this assignment can be made private, if it's overridden for thread-test.o, not thread-test. So the final patch is:

diff --git a/src/Makefile b/src/Makefile
index 0e423c4..1c0021b 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -125,8 +125,8 @@ efisecdb-static : | $(GENERATED_SOURCES)
 efisecdb-static : LIBS=dl
 
 thread-test : libefivar.so
-thread-test : CFLAGS=$(HOST_CFLAGS) -I$(TOPDIR)/src/include/efivar
-thread-test : LIBS=pthread efivar
+thread-test.o : private CFLAGS=$(HOST_CFLAGS) -I$(TOPDIR)/src/include/efivar
+thread-test : private LIBS=pthread efivar
 
 deps : $(ALL_SOURCES)
        @$(MAKE) -f $(SRCDIR)/include/deps.mk deps SOURCES="$(ALL_SOURCES)"

[trofi]

Oh, that's a lot trickier and way above my understanding of target-specific variables.

Thank you!

I'll try to read through Target-specific Variable Values section of GNU make and will adapt the PR to use your approach.

[trofi]

Yeah, I agree that in presence of target-specific variables --shuffle is able to schedule commands for execution in a way not reachable with a default make -j. --shuffle produces different (but still valid) topological traversals of build graph.

For example for Makefile like the following:

a: a.o c.o
a: CFLAGS += -DFOO

b: b.o c.o
b: CFLAGS += -DBAR

AFAIU there is no way to force make a b -j to build c.o with -DBAR without rearranging them in command-line or without changing the Makefile. It is still worth making thing consistent in light of make b a call, but trickier to illustrate the practical problem.

I'll update the PR tomorrow.

[trofi]

Got slightly better understanding how local variable overrides leak into prerequisites. Updated the PR and PR description on efisecdb example.

[trofi]

Drew a few pictures to explain the problem behind the PR a bit better: https://trofi.github.io/posts/294-an-obscure-make-shuffle-bug.html

[JakMobius]

I'm glad to help, thank you for the mention!