Conversation
makint the CLI ons step closer to being linkable to the dynamic library
invoking target zstd-dll
| $(CC) $(FLAGS) $^ -o $@$(EXT) $(LDFLAGS) | ||
| zstd-dll : LDFLAGS+= -L$(ZSTDDIR) | ||
| zstd-dll : LDLIBS += -lzstd | ||
| zstd-dll : ZSTDLIB_LOCAL_SRC = xxhash.c |
There was a problem hiding this comment.
I'm not sure this is needed. When I looked, it appeared that our default -fvisibility=hidden was not applied to xxhash symbols in libzstd.so at all. (Maybe that is a bug.)
There was a problem hiding this comment.
It is not the intended visibility anyway
There was a problem hiding this comment.
$ nm -D lib/libzstd.so.1.4.8 | grep XXH
000000000000f7c0 T ZSTD_XXH32
0000000000010360 T ZSTD_XXH32_canonicalFromHash
000000000000f760 T ZSTD_XXH32_copyState
000000000000fc20 T ZSTD_XXH32_createState
000000000000feb0 T ZSTD_XXH32_digest
000000000000fc30 T ZSTD_XXH32_freeState
0000000000010380 T ZSTD_XXH32_hashFromCanonical
000000000000fc60 T ZSTD_XXH32_reset
000000000000fd00 T ZSTD_XXH32_update
000000000000f930 T ZSTD_XXH64
0000000000010370 T ZSTD_XXH64_canonicalFromHash
000000000000f780 T ZSTD_XXH64_copyState
000000000000fc40 T ZSTD_XXH64_createState
0000000000010140 T ZSTD_XXH64_digest
000000000000fc50 T ZSTD_XXH64_freeState
0000000000010390 T ZSTD_XXH64_hashFromCanonical
000000000000fca0 T ZSTD_XXH64_reset
000000000000ff80 T ZSTD_XXH64_update
000000000000f750 T ZSTD_XXH_versionNumber
Oops :)
There was a problem hiding this comment.
I tested 1.4.8 on a recent Ubuntu station,
and observed the same issue : all symbols, including internal ones, seem to be visible
(note: I assume that a symbol name prefixed by T means "visible", while t means "not visible").
What's strange is that I also checked the compilation line, and the link stage does include the -fvisibility=hidden directive. So visibility was supposed to be more selective.
However, we have recently changed the build system to separate compilation and link stage, in order to offer incremental build for faster development iteration. -fvisibility=hidden is not part of compilation (into *.o), only part of link stage.
I tested making it part of compilation too, and it seems to fix the visibility issue.
Which seems strange to me, because it seems to contradict all documentation I could read about this feature (and re-checked today), which was labelled "link stage only". So I'm no longer sure what's the "ideal" solution here.
That's probably an issue to fix in another PR.
terrelln
left a comment
terrelln
left a comment
There was a problem hiding this comment.
The changes look good to me!
I do have a question about the objective. Do we want to officially support linking the zstd CLI against the dynamic library? If so, do we want to only support CLI version == lib version? Should we add a version check the the CLI that checks ZSTD_VERSION_NUMBER == ZSTD_versionNumber()?
I would like that, if it isn't too much additional burden.
I would be happy with this restriction. I don't know to what extent we think about ABI compatibility and symbol versioning in the libzstd shared library today (for non-CLI consumers). The proposed implementation of Thanks, folks! |
We keep ABI compatibility for the "stable" portion of our API (everything that isn't hidden behind |
|
With these changes, the Because experimental symbols and capabilities can change from one version to another, it's indeed safer to lock the library version. One thing which I'm not completely sure of is : I presume it may depend on the way library symbols are linked in the program : lazy evaluation should defer the discovery of the missing symbol up to the moment it's needed. Therefore, there is time for a runtime check. |
|
There is a remaining test error, but it's unrelated :
|
4 participants
supersedes #2456
and possibly #2450