-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698
Conversation
mk/libraries.mk
Outdated
@@ -127,7 +127,7 @@ define build-library | |||
$$($(1)_PATH): $$($(1)_OBJS) | $$(_d)/ | |||
$(trace-ar) $(AR) crs $$@ $$? | |||
|
|||
$(1)_LDFLAGS_USE += $$($(1)_PATH) $$($(1)_LDFLAGS) | |||
$(1)_LDFLAGS_USE += -Wl,--whole-archive $$($(1)_PATH) -Wl,--no-whole-archive $$($(1)_LDFLAGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why first one then the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only want the whole archive of the local Nix libraries like libnixstore.a. So this enables --whole-archive
for one .a file then immediately disables it. For most dependencies, we only want to include the symbols actually used by the binary. If we enabled --whole-archive
for all libraries, we would end up with tons of unused symbols from libcurl, aws, boehm-gc, etc.
The one case where we need --whole-archive
is for libnixstore.a which has an .init
elf section we need to keep. This comes from the reliance on C++ Global Constructors feature. Some references on this:
e521c09
to
6587ffd
Compare
@edolstra @grahamc Could you reconsider this one for nix-2.3? It is necessary to do things like https://matthewbauer.us/blog/static-nix.html |
Doesn't this cause the |
Yeah, but mostly only because those symbols are missing.
To make things smallers, I've added a flag that just applies it for libstore.a. It doesn't help too much though:
Try:
Notice that the binary cache isn't working, because the regStore symbols aren't linked into the executable! |
I also should note this just effects |
|
I don't think there is. https://github.com/matthewbauer/nix/tree/fix-static-linking-used It is still missing the necessary symbols. Here is a more concise sample that shows what happens: Two alternatives are possible:
Both of these end up special casing static compilation in awkward ways. The second one is especially bad because we need to update it every time we add a new instance of RegisterStoreImplementation. I still think |
Do you think that we can go forward with this PR somehow? I would love to see a recent static nix version. :) |
Would it make sense to proceed for now with |
This comment has been minimized.
This comment has been minimized.
I also hit this issue with a statically-linked |
For reference, |
Is store-api.hh supposed to support store implementations that are not part of the nix code base? Otherwise just having a list of initializer would be simple and straightforward. The only other way to support static linking with those initializer would be a linker script would be some sort of linker script. |
All classes that make use of static initializers:
I think as long as each object file contains at least one exported symbol that is linked, their static initializers are loaded as well. |
I opened #3511 as an alternative fix. |
I think plugins are allowed to provide their own implementation. |
When statically linking, symbols that are not referenced are not included in the final executable. Usually, this is an okay optimization, but sometimes can cause unexpected issues. In this case, statically linked Nix was not finding the HttpBinaryCacheStore class and the RegisterStoreImplementation that comes with it. This is because the resulting binary would not run all of the store registration at initialization time. Some of the store registrations were still heppning like in the S3 case, but only because another symbol in s3-binary-cache-store.o was being used elsewhere in Nix. This linking behavior caused static Nix to have errors like: don't know how to open Nix store ‘https://...’ Adding --whole-archive forces the linker to include these archives. This is just applied to the local Nix libraries, and not external libraries. As a side effect we might get some symbols that we don’t actually need, but that is the same case as with dynamically linked Nix.
I forgot to verify this on macOS. We need to use -Wl,-force_load here.
These also need it: - NixOS#2698 (comment) - NixOS#2698 (comment)
cc530c2
to
c4794a6
Compare
Since upstream is not interested in a fix for this problem, should we have public fork that also provides static binaries for download? nix-community might be a good place. |
It's not that I don't care about a fix, but it seems that the real bug here is not running static initializers. I'm not sure but I would think that C++ requires them to be executed. |
This issue is not fixable on a language level without refactorings like #3511. |
@Mic92 If a compiler has an output mode that doesn't respect the semantics of the language, then it's a bug in the compiler. |
I think the big question is whether C++ allows unused static initializers to be subject to dead code elimination. Someone should try to build Nix dynamically with Another thing to try is: instead of linking the executable from compiled sources, directly compile and link the source in one go (don't set |
This behavior is specified, or at least allowed, by the C++ standard (if I'm reading it correctly):
That is, if you do not use a source file at all, the compiler is free to forego initialization. |
Closing in favor of #2698 since i think that is a better approach. It makes static and dynamic linking closer to each other. |
I think you meant to reference pr 3677 ? |
When statically linking, symbols that are not referenced are not
included in the final executable. Usually, this is an okay
optimization, but sometimes can cause unexpected issues.
In this case, statically linked Nix was not finding the
HttpBinaryCacheStore class and the RegisterStoreImplementation that
comes with it. This is because the resulting binary would not run all
of the store registration at initialization time. Some of the store
registrations were still heppning like in the S3 case, but only
because another symbol in s3-binary-cache-store.o was being used
elsewhere in Nix. This linking behavior caused static Nix to have
errors like:
don't know how to open Nix store ‘https://...’
Adding --whole-archive forces the linker to include these archives.
This is just applied to the local Nix libraries, and not external
libraries. As a side effect we might get some symbols that we don’t
actually need, but that is the same case as with dynamically linked
Nix.
You can try out my statically built pr here:
NixOS/nixpkgs#56281