Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: consider linking the CLI tool against the shared library #2976

Open
eli-schwartz opened this issue Jan 5, 2022 · 4 comments
Open

RFC: consider linking the CLI tool against the shared library #2976

eli-schwartz opened this issue Jan 5, 2022 · 4 comments

Comments

@eli-schwartz
Copy link
Contributor

Split out from #2950

Currently, the official Makefiles statically link the CLI tool zstd against the library objects. This has an immediately crucial purpose: the tool links against some private symbols.

It would be interesting to consider optionally linking against the shared library instead. The private symbols can be refactored into an internal utility library called, for the sake of argument, libcommon.a, and linked to both the shared library and to the program(s) that need it.

This has a couple of advantages:

  • keep installation size down, which isn't a huge deal in most cases (but use cases vary)
  • distros like it due to $REASONS
  • for code organization purposes, it helps to distinguish between what is expected to be used by internal utilities, and what isn't -- and avoid possibly leaking the entirety of libzstd's internals to the in-tree binaries
  • it may occasionally find code that doesn't belong there and wasn't supposed to do anything, but nevertheless does, as in Building with -O0 causes programs/zstd to fail to link #2950 :D

An existing implementation of this concept is already present in meson.build -- and this originally worked because meson didn't implement the -fvisibility settings the Makefile did. I fixed that, and in the process had to unpack the objects from the shared library and link them individually to the CLI tool and to test utilities.

Meson's built-in option -Ddefault_library={static|shared|both} determines whether to build the shared and/or static libraries, and then the program will be linked to whichever the user chose.

However, it becomes a bit complicated and undocumented to figure out which object files need to be linked in on top of libzstd itself. Making this officially supported in the Makefile build would be neat.

@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Apr 28, 2022

Seems like in git dev building on MSVC is broken due to

[86/92] Linking target subprojects/zstd-fix3119/build/meson/programs/zstd.exe
FAILED: subprojects/zstd-fix3119/build/meson/programs/zstd.exe subprojects/zstd-fix3119/build/meson/programs/zstd.pdb 
"link"  /MACHINE:x64 /OUT:subprojects/zstd-fix3119/build/meson/programs/zstd.exe subprojects/zstd-fix3119/build/meson/programs/subprojects_zstd-fix3119_build_meson_programs_.._.._.._build_VS2010_zstd_zstd.rc_zstd.res subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_zstdcli.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_util.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_timefn.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_fileio.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_fileio_asyncio.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_benchfn.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_benchzstd.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_datagen.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_dibio.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._programs_zstdcli_trace.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._lib_common_xxhash.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._lib_common_pool.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._lib_common_zstd_common.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd.exe.p/.._.._.._lib_common_error_private.c.obj "/nologo" "/release" "/nologo" "/DEBUG" "/PDB:subprojects\zstd-fix3119\build/meson/programs\zstd.pdb" "subprojects\zstd-fix3119\build/meson/lib\zstd.lib" "/SUBSYSTEM:CONSOLE" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
.._.._.._lib_common_pool.c.obj : error LNK2019: unresolved external symbol ZSTD_pthread_create referenced in function POOL_create_advanced
.._.._.._lib_common_pool.c.obj : error LNK2019: unresolved external symbol ZSTD_pthread_join referenced in function POOL_join
subprojects\zstd-fix3119\build\meson\programs\zstd.exe : fatal error LNK1120: 2 unresolved externals
[87/92] Linking target subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe
FAILED: subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.pdb 
"link"  /MACHINE:x64 /OUT:subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._programs_zstdcli.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._programs_timefn.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._programs_util.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._programs_fileio.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._programs_fileio_asyncio.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._lib_common_pool.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._lib_common_zstd_common.c.obj subprojects/zstd-fix3119/build/meson/programs/zstd-frugal.exe.p/.._.._.._lib_common_error_private.c.obj "/nologo" "/release" "/nologo" "/DEBUG" "/PDB:subprojects\zstd-fix3119\build/meson/programs\zstd-frugal.pdb" "subprojects\zstd-fix3119\build/meson/lib\zstd.lib" "/SUBSYSTEM:CONSOLE" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
.._.._.._lib_common_pool.c.obj : error LNK2019: unresolved external symbol ZSTD_pthread_create referenced in function POOL_create_advanced
.._.._.._lib_common_pool.c.obj : error LNK2019: unresolved external symbol ZSTD_pthread_join referenced in function POOL_join
subprojects\zstd-fix3119\build\meson\programs\zstd-frugal.exe : fatal error LNK1120: 2 unresolved externals
[88/92] Linking target subprojects/zstd-fix3119/build/meson/tests/checkTag.exe
ninja: build stopped: subcommand failed.

This happens, apparently, because these executables need lib/common/threading.c symbols but they aren't globally available. Interesting to note, this happened in dev after the 1.5.2 release?

I have an experimental fix that applies the private symbols more reliably after the shared library symbols get resolved: 1aaa10c

Unfortunately:

  • my experimental fix means less maintenance on Linux, but it currently works there anyway, so it's all good, no bug fix but an optimization
  • it seems to be quite broken on MSVC, which is what I wanted it for

GCC allows linking to both the shared and static libraries, and it resolves symbols from the shared library first, and then pulls in the implementations of still-missing symbols from the static library. MSVC errors out with:

libzstd_objlib.a(.._.._.._lib_common_zstd_common.c.obj) : error LNK2005: ZSTD_getErrorCode already defined in zstd.lib(zstd-1.dll)
libzstd_objlib.a(.._.._.._lib_common_zstd_common.c.obj) : error LNK2005: ZSTD_versionString already defined in zstd.lib(zstd-1.dll)
libzstd_objlib.a(.._.._.._lib_common_zstd_common.c.obj) : error LNK2005: ZSTD_isError already defined in zstd.lib(zstd-1.dll)
libzstd_objlib.a(.._.._.._lib_common_zstd_common.c.obj) : error LNK2005: ZSTD_getErrorName already defined in zstd.lib(zstd-1.dll)

I don't know enough about MSVC to know if you can tell it "yes I know, but given a choice between an import library and a static library, choose the version in the import library and don't complain".

I'm not sure whether there is a reliable solution here other than:

  • on MSVC, just give up and always link to a static library, most likely even Windows users who want to build DLLs don't want to juggle them for the tools
  • actually do move the private symbols into the internal utility library I mentioned above

@Cyan4973
Copy link
Contributor

I believe this dependency was added by @yoniko , to implement asynchronous decompression feature in the CLI.
Indeed, this feature is not part of any release, hence not part of v1.5.2.

I also think we already stated that we don't support linking the CLI to the dynamic library, due to usage of non-stable (experimental) symbols. We don't foresee a situation where we would limit the CLI to stable library symbols only, so this will remain the policy for the foreseeable future.

@eli-schwartz
Copy link
Contributor Author

Maybe it would help if I rephrased the problem a bit.

Linking the CLI to the dynamic library (in a build configuration where the library is dynamic, not static) is useful because it reduces code duplication in the installed product. It doesn't imply that only public symbols may be used, but it does mean that symbols which are public will be gotten from the dynamic library instead of included from the static library.

I have successfully done that for the most part in the meson.build, and I have a slightly optimized hack to improve its accuracy. However, I don't know how to do it for MSVC, or if it's even possible, so I have also implemented completely disabling the use of dynamic symbols from zstd.dll on MSVC.

This could be made properly reliable, if zstd produced two libraries internally:

  • the public zstd library, which only contains public symbols, and links to...
  • a private "libcommon" library which contains all private symbols which are used by both the library and by CLI tools

Inside the build system, the zstd library would:

  • if built shared, link to libcommon, and embed private symbols without exposing them
  • if built static, link to libcommon by including all of libcommon's objects (Meson supports this by default)

Then tools could link to both zstd-shared and libcommon-static, and share symbols with the dynamic library where possible, but otherwise behave as they currently do.

The straightforward way of implementing such a thing would be to just collect all public symbols into one set of files, and all private symbols into another set of files. Then just always link the tools to both libraries. That is how I'd write it for Meson.

eli-schwartz added a commit to eli-schwartz/zstd that referenced this issue Apr 29, 2022
…f it

Partial, Meson-only implementation of facebook#2976 for non-MSVC builds.

Due to the prevalence of private symbol reuse, linking to a shared
library is simply utterly unreliable, but we still want to defer to the
shared library for installable applications. By linking to both, we can
share symbols where possible, and statically link where needed.

This means we no longer need to manually track every file that needs to
be extracted and reused.

The flip side is that MSVC completely does not support this, so for MSVC
builds we just link to a full static copy even where
-Ddefault_library=shared.

As a side benefit, by using library inclusion rather than including
extra explicit object files, the zstd program shrinks in size slightly
(~4kb).
@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Apr 29, 2022

The PR I just linked above, fixes building on MSVC (tests still fail, cf. my other PR) by completely disabling the "linking the CLI tool against the shared library" trick that Meson historically did, but only for MSVC.

I also anticipate it should entirely prevent future occurrences of "private symbols moved around and now the contributed Meson files fail to build because it does something we don't officially support". I remain convinced that the Meson files should do the "right thing" and link to the shared library, but it will no longer be at the cost of fragility in tracking private symbols.

eli-schwartz added a commit to eli-schwartz/zstd that referenced this issue Apr 29, 2022
…f it

Partial, Meson-only implementation of facebook#2976 for non-MSVC builds.

Due to the prevalence of private symbol reuse, linking to a shared
library is simply utterly unreliable, but we still want to defer to the
shared library for installable applications. By linking to both, we can
share symbols where possible, and statically link where needed.

This means we no longer need to manually track every file that needs to
be extracted and reused.

The flip side is that MSVC completely does not support this, so for MSVC
builds we just link to a full static copy even where
-Ddefault_library=shared.

As a side benefit, by using library inclusion rather than including
extra explicit object files, the zstd program shrinks in size slightly
(~4kb).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants