'make -C lib lib-mt' produces underlinked libzstd.so

[trofi]

Original build failure was reported as a gcc-10 build failure when zstd is installed in system. Shorter version of underlying cause is https://bugs.gentoo.org/713940.

The simplest way to reproduce it is to try to build multi-threaded libzstd.so with undefined symbols forbidden (there should be no undefined symbols in non-plugin libraries anyway).

I injected -Wl,--no-undefined manually as I don't see a nice way to append to existing LDFLAGS:

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -180,11 +180,13 @@ endif
 libzstd : $(LIBZSTD)

 libzstd-mt : CPPFLAGS += -DZSTD_MULTITHREAD
+libzstd-mt : LDFLAGS  += -Wl,--no-undefined
 libzstd-mt : libzstd

 lib: libzstd.a libzstd

 lib-mt: CPPFLAGS += -DZSTD_MULTITHREAD
+lib-mt: LDFLAGS  += -Wl,--no-undefined
 lib-mt: lib

 lib-release lib-release-mt: DEBUGFLAGS :=

Now build failure shows underlinked library:

$ LANG=C make -C lib lib-mt
compiling dynamic library 1.4.5
...
/usr/lib/gcc/x86_64-pc-linux-gnu/10.0.1/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccC81zhY.o: in function `POOL_free.part.0':
pool.c:(.text+0x16f): undefined reference to `pthread_join'
/usr/lib/gcc/x86_64-pc-linux-gnu/10.0.1/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccC81zhY.o: in function `POOL_create_advanced':
pool.c:(.text+0x322): undefined reference to `pthread_create'
/usr/lib/gcc/x86_64-pc-linux-gnu/10.0.1/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccC81zhY.o: in function `POOL_resize':
pool.c:(.text+0x4e1): undefined reference to `pthread_create'
collect2: error: ld returned 1 exit status
make: *** [Makefile:172: libzstd.so.1.4.5] Error 1

Desktop (please complete the following information):

• OS: Linux
• Compiler: gcc-10
• Flags: defaults, multithreaded
• Build system: Makefile

Reading https://github.com/facebook/zstd/blob/dev/lib/README.md#multithreading-support it looks like build system is supposed to pass -pthread when building/linking libzstd.so, but it does not:

Multithreading support

Multithreading is disabled by default when building with make. Enabling multithreading requires 2 conditions :

    set build macro ZSTD_MULTITHREAD (-DZSTD_MULTITHREAD for gcc)
    for POSIX systems : compile with pthread (-pthread compilation flag for gcc)

Both conditions are automatically applied when invoking make lib-mt target.

...

The following fix seems to be enough to fix underlinking:

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -187,6 +187,8 @@ lib: libzstd.a libzstd
 lib-mt: CPPFLAGS += -DZSTD_MULTITHREAD
 lib-mt: lib

+lib-mt: LDFLAGS  += -pthread
+
 lib-release lib-release-mt: DEBUGFLAGS :=
 lib-release: lib
 lib-release-mt: lib-mt

[Cyan4973]

Thanks @trofi for this very interesting report.

I was surprised to read, through the comments, that underlinking is a thing, which may even be achieved intentionally (samba is mentioned for this case), though the use case seems complex (loaded executable exported with -rdynamic).

Most other docs on this topic advise not to underlink libraries. There are additional impacts on pkgconfig, as library must rather define its dependencies into Libs.private:, rather than the more common Libs:, to avoid impacting library users.

It's interesting as it seems to open a way towards enabling multi-threading in libzstd : so far, it wasn't enabled by default, on the ground that all user programs would need to add -pthread as part of their build script, which would be a breaking change. But if I understand correctly, after this underlinking issue is fixed, adding -pthread in the user build script is no longer necessary, provided that the user program doesn't need -pthread for its own purpose. If that analysis is correct, it would be a great step forward.

First thing to do : update tests, so that they can catch this situation. Then apply fix.

Minor detail : I'm not sure how "universal" -Wl,--no-undefined is. What if CC is clang, or an old version of gcc, or even icc ? To avoid incompatibilities, it probably belongs to some kind of LD_DEBUGFLAGS, that would be always run in CI in order to catch under-linking issues, but is not required for local production of the library (same process as other warning flags for CFLAGS).

[trofi]

Thanks @trofi for this very interesting report.

I was surprised to read, through the comments, that underlinking is a thing, which may even be achieved intentionally (samba is mentioned for this case), though the use case seems complex (loaded executable exported with -rdynamic).

Most other docs on this topic advise not to underlink libraries. There are additional impacts on pkgconfig, as library must rather define its dependencies into Libs.private:, rather than the more common Libs:, to avoid impacting library users.

Correct. That matches my understanding.

It's interesting as it seems to open a way towards enabling multi-threading in libzstd : so far, it wasn't enabled by default, on the ground that all user programs would need to add -pthread as part of their build script, which would be a breaking change. But if I understand correctly, after this underlinking issue is fixed, adding -pthread in the user build script is no longer necessary, provided that the user program doesn't need -pthread for its own purpose. If that analysis is correct, it would be a great step forward.

Correct. That also matches my understanding. pthread can become just an implementation detail.

First thing to do : update tests, so that they can catch this situation. Then apply fix.

Minor detail : I'm not sure how "universal" -Wl,--no-undefined is. What if CC is clang, or an old version of gcc, or even icc ? To avoid incompatibilities, it probably belongs to some kind of LD_DEBUGFLAGS, that would be always run in CI in order to catch under-linking issues, but is not required for local production of the library (same process as other warning flags for CFLAGS).

-Wl, is a way to pass --no-undefined directly to linker. All of ld.bfd, ld.gold and ld.lld do implement --no-undefined. But I agree, it's suport should be autodetected for less popular linkers.

[Cyan4973]

for information, tested with macos-x 's clang :

ld: unknown option: --no-undefined 
clang: error: linker command failed with exit code 1 (use -v to see invocation)

note : clang on linux works fine, so it's not just a matter of compiler, but also system