make / cmake inconsistencies

[patrickdepinguin]

Describe the bug
The compiler and link commands used depend on the build system used, e.g. make vs cmake (maybe same for the other build systems). This is unexpected and undesired.

For example, for libzstd, the makefile specifies -fvisibility=hidden, whereas the cmake version does not.
Similarly, the compilation of the 'programs' have many compiler warning flags which are absent for cmake.
On the other hand, the cmake system now allows to link the 'zstd' program dynamically to libzstd via -DZSTD_PROGRAMS_LINK_SHARED=ON, but this feature is not supported in the 'make' system. There does exist a 'zstd-dll' target, which does not work because the visibility of symbols in libzstd is set to hidden.

To Reproduce
Steps to reproduce the behavior:

• compare output from a clean directory for following steps:
  • cmake build/cmake/ -j1 V=1 VERBOSE=1
  • make -j1 V=1 VERBOSE=1

Expected behavior
I expect the same output files regardless of the build system

Additional context
With the upgrade from zstd 1.4.3 to 1.4.5, I notice yet another big increase in size, +500KB on libzstd. I thus wanted to benefit from ZSTD_PROGRAMS_LINK_SHARED=ON, but zstd as packaged in Buildroot is using 'make', hence I stumbled upon these inconsistencies.

[patrickdepinguin]

I'm currently using this patch to support ZSTD_PROGRAMS_LINK_SHARED in the make context.

From afc368e7247abfe89250def5b7673a9ccb79e0b1 Mon Sep 17 00:00:00 2001
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Date: Wed, 5 Aug 2020 15:35:01 +0200
Subject: [PATCH] make: support ZSTD_PROGRAMS_LINK_SHARED to link zstd with
 libzstd

zstd allows building via make, cmake, meson, and others, but unfortunately
the features and flags used for these build systems are inconsistent.

The cmake version respects an option 'ZSTD_PROGRAMS_LINK_SHARED' to let the
zstd binary link dynamically with its own libzstd. Without it, the zstd
program uses static linking and thus has a big size.

This option is not provided in the 'make' system. There does exist a target
'zstd-dll' which intends to do exactly this, but it does not work out of the
box due to linking issues. These in turn are caused because the lib makefile
passes '-fvisibility=hidden' to the linker, which hides certain symbols that
the zstd program needs. The cmake-based compilation does not pass this
visibility flag, due to which all symbols are visible.

Unfortunately, there is no 'install' rule that will install zstd-dll and
create the derived programs like zstdless etc.

With the above in mind, when ZSTD_PROGRAMS_LINK_SHARED is set in make
context:
- remove '-fvisibility=hidden' from the lib compilation
- alter the 'zstd' target to not include the lib objects directly but link
  dynamically with libzstd.

See also https://github.com/facebook/zstd/issues/2261 which reported these
inconsistencies between make and cmake compilation.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

---
 lib/Makefile      | 5 ++++-
 programs/Makefile | 8 +++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 7c6dff02..8f9f36a6 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -204,7 +204,10 @@ $(LIBZSTD): $(ZSTD_FILES)
 else
 
 LIBZSTD = libzstd.$(SHARED_EXT_VER)
-$(LIBZSTD): LDFLAGS += -shared -fPIC -fvisibility=hidden
+$(LIBZSTD): LDFLAGS += -shared -fPIC
+ifndef ZSTD_PROGRAMS_LINK_SHARED
+$(LIBZSTD): LDFLAGS += -fvisibility=hidden
+endif
 $(LIBZSTD): $(ZSTD_FILES)
 	@echo compiling dynamic library $(LIBVER)
 	$(Q)$(CC) $(FLAGS) $^ $(LDFLAGS) $(SONAME_FLAGS) -o $@
diff --git a/programs/Makefile b/programs/Makefile
index 418ad4e6..8897dcf9 100644
--- a/programs/Makefile
+++ b/programs/Makefile
@@ -169,10 +169,16 @@ $(ZSTDDECOMP_O): CFLAGS += $(ALIGN_LOOP)
 zstd : CPPFLAGS += $(THREAD_CPP) $(ZLIBCPP) $(LZMACPP) $(LZ4CPP)
 zstd : LDFLAGS += $(THREAD_LD) $(ZLIBLD) $(LZMALD) $(LZ4LD) $(DEBUGFLAGS_LD)
 zstd : CPPFLAGS += -DZSTD_LEGACY_SUPPORT=$(ZSTD_LEGACY_SUPPORT)
+ifdef ZSTD_PROGRAMS_LINK_SHARED
+# link dynamically against own library
+zstd : LDFLAGS += -L$(ZSTDDIR) -lzstd
+else
+zstd : $(ZSTDLIB_FILES)
+endif
 ifneq (,$(filter Windows%,$(OS)))
 zstd : $(RES_FILE)
 endif
-zstd : $(ZSTDLIB_FILES) $(ZSTD_CLI_OBJ)
+zstd : $(ZSTD_CLI_OBJ)
 	@echo "$(THREAD_MSG)"
 	@echo "$(ZLIB_MSG)"
 	@echo "$(LZMA_MSG)"
-- 
2.26.2

[cemeyer]

Here is a slightly different proposal. Rather than dropping -fvisibility=hidden entirely, just expose the two hidden symbols needed to link zstd:

commit fac6b61b
Author: Conrad Meyer <cem@FreeBSD.org>
Date:   Sun Jan 3 07:12:24 2021 -0800

    make: Add ZSTD_PROGRAMS_LINK_SHARED to dll-link zstd
    
    Add a make variable ZSTD_PROGRAMS_LINK_SHARED.  When set, libzstd
    exports private symbols needed to shared-link the zstd program.  zstd is
    linked against this libzstd.so.

diff --git a/lib/Makefile b/lib/Makefile
index c9bd4244..a0fab3b0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -60,6 +60,9 @@ CPPFLAGS += -DXXH_NAMESPACE=ZSTD_ -DDEBUGLEVEL=$(DEBUGLEVEL)
 ifeq ($(TARGET_SYSTEM),Windows_NT)   # MinGW assumed
   CPPFLAGS += -D__USE_MINGW_ANSI_STDIO   # compatibility with %zu formatting
 endif
+ifdef ZSTD_PROGRAMS_LINK_SHARED
+  CPPFLAGS += -DZSTD_PROGRAMS_LINK_SHARED
+endif
 DEBUGFLAGS= -Wall -Wextra -Wcast-qual -Wcast-align -Wshadow \
             -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement \
             -Wstrict-prototypes -Wundef -Wpointer-arith \
diff --git a/lib/compress/zstd_compress_internal.h b/lib/compress/zstd_compress_internal.h
index c04998b8..0a703245 100644
--- a/lib/compress/zstd_compress_internal.h
+++ b/lib/compress/zstd_compress_internal.h
@@ -1198,6 +1198,9 @@ size_t ZSTD_referenceExternalSequences(ZSTD_CCtx* cctx, rawSeq* seq, size_t nbSe
 
 /** ZSTD_cycleLog() :
  *  condition for correct operation : hashLog > 1 */
+#if defined(ZSTD_PROGRAMS_LINK_SHARED)
+ZSTDLIB_API
+#endif
 U32 ZSTD_cycleLog(U32 hashLog, ZSTD_strategy strat);
 
 #endif /* ZSTD_COMPRESS_H */
diff --git a/lib/dictBuilder/zdict.c b/lib/dictBuilder/zdict.c
index 79c522ef..aaf0b288 100644
--- a/lib/dictBuilder/zdict.c
+++ b/lib/dictBuilder/zdict.c
@@ -968,6 +968,9 @@ static size_t ZDICT_addEntropyTablesFromBuffer_advanced(
 }
 
 /* Hidden declaration for dbio.c */
+#if defined(ZSTD_PROGRAMS_LINK_SHARED)
+ZSTDLIB_API
+#endif
 size_t ZDICT_trainFromBuffer_unsafe_legacy(
                             void* dictBuffer, size_t maxDictSize,
                             const void* samplesBuffer, const size_t* samplesSizes, unsigned nbSamples,
diff --git a/programs/Makefile b/programs/Makefile
index e66d182f..e983e03e 100644
--- a/programs/Makefile
+++ b/programs/Makefile
@@ -213,6 +213,10 @@ zstd : CPPFLAGS += $(THREAD_CPP) $(ZLIBCPP) $(LZMACPP) $(LZ4CPP)
 zstd : LDFLAGS += $(THREAD_LD) $(DEBUGFLAGS_LD)
 zstd : LDLIBS += $(ZLIBLD) $(LZMALD) $(LZ4LD)
 zstd : CPPFLAGS += -DZSTD_LEGACY_SUPPORT=$(ZSTD_LEGACY_SUPPORT)
+ifdef ZSTD_PROGRAMS_LINK_SHARED
+zstd : LDFLAGS+= -L$(ZSTDDIR) -lzstd
+zstd : ZSTDLIB_FULL_SRC =
+endif
 ifneq (,$(filter Windows%,$(OS)))
 zstd : $(RES_FILE)
 endif
@@ -226,7 +230,11 @@ zstd:
 else
 # BUILD_DIR is defined
 
+ifndef ZSTD_PROGRAMS_LINK_SHARED
 ZSTD_OBJ := $(addprefix $(BUILD_DIR)/, $(ZSTD_ALL_OBJ))
+else
+ZSTD_OBJ := $(addprefix $(BUILD_DIR)/, $(ZSTD_CLI_OBJ))
+endif
 $(BUILD_DIR)/zstd : $(ZSTD_OBJ)
 	@echo "$(THREAD_MSG)"
 	@echo "$(ZLIB_MSG)"
@@ -288,20 +296,6 @@ zstd-noxz : LZMALD  :=
 zstd-noxz : LZMA_MSG := - xz/lzma support is disabled
 zstd-noxz : zstd
 
-## zstd-dll: zstd executable linked to dynamic library libzstd (must already exist)
-# note : the following target doesn't link
-#        because zstd uses non-public symbols from libzstd
-#        such as XXH64 (for benchmark),
-#        ZDICT_trainFromBuffer_unsafe_legacy (for dictionary builder)
-#        and ZSTD_cycleLog (likely for --patch-from).
-#        It's unclear at this stage if this is a scenario that must be supported
-.PHONY: zstd-dll
-zstd-dll : LDFLAGS+= -L$(ZSTDDIR) -lzstd
-zstd-dll : ZSTDLIB_FULL_SRC =
-zstd-dll : $(ZSTD_CLI_OBJ)
-	$(CC) $(FLAGS) $^ -o $@$(EXT) $(LDFLAGS)
-
-
 ## zstd-pgo: zstd executable optimized with PGO.
 zstd-pgo :
 	$(MAKE) clean

The unuseful zstd-dll target is discarded. I plan to do something similar in FreeBSD (which has an independent reimplementation of zstd's build).

(Something similar could be done for Cmake build, which does not appear to set visibility symbols at all. I assume BUCK build does not care for dynamically-linked zstd and we can leave it alone.)

See #1680 , #1854 for prior interest in shared-linked zstd.

[patrickdepinguin]

@cemeyer I tried your patch on top of zstd 1.4.5 (had to adapt a little because meanwhile the makefiles changed a bit).
But I get a compilation failure in the bench code:

==> building with threading support
==> building zstd with .gz compression support
==> no liblzma, building zstd without .xz/.lzma support
==> no liblz4, building zstd without .lz4 support
.../buildroot/output/host/bin/mips64-octeon-linux-gnu-gcc -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -DXXH_NAMESPACE=ZSTD_ -DZSTD_MULTITHREAD -DZSTD_GZCOMPRESS -DZSTD_GZDECOMPRESS   -DZSTD_LEGACY_SUPPORT=5 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os -g2  -Wall -Wextra -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wpointer-arith -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wmissing-prototypes -Wc++-compat  -pthread -lz    -L../lib -lzstd benchfn.o benchzstd.o datagen.o dibio.o fileio.o timefn.o util.o zstdcli.o -o zstd -pthread -lz    -L../lib -lzstd
benchzstd.o: In function `BMK_benchMemAdvancedNoAlloc':
.../buildroot/output/build/zstd-1.4.5/programs/benchzstd.c:393: undefined reference to `ZSTD_XXH64'
.../buildroot/output/build/zstd-1.4.5/programs/benchzstd.c:497: undefined reference to `ZSTD_XXH64'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:189: zstd] Error 1
make[2]: Leaving directory '.../buildroot/output/build/zstd-1.4.5/programs'
make[1]: *** [Makefile:63: zstd] Error 2

With my patch I don't have this problem, although I couldn't immediately find out how to fix it while keeping true to your method.
Are you building this code too, or are you building a minimal set?

[cemeyer]

The patch built on 1.4.8 because the intended symbol visibility was not being applied correctly. I don’t know if it was working in 1.4.5.

It was fixed again in dev by #2441 .

Dev branch has a way to build dll-linked cli now, I would just use that instead.

[patrickdepinguin]

Great, the 'zstd-dll' target does match what I need. Looking forward to the next release...

[eli-schwartz]

The linked PR fixes symbol visibility for meson.