Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698

matthewbauer · 2019-02-24T07:49:40Z

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

grahamc · 2019-05-17T01:15:55Z

mk/libraries.mk

@@ -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)


why first one then the other?

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:

http://l4u-00.jinr.ru/usoft/WWW/www_debian.org/Documentation/elf/node4.html#SECTION00031000000000000000

cgo: C++ static initialization not working bazelbuild/rules_go#1486

https://www.bfilipek.com/2018/02/static-vars-static-lib.html

matthewbauer · 2019-08-28T19:12:20Z

@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

edolstra · 2019-08-28T19:55:45Z

Doesn't this cause the nix executable to be potentially much larger than it should be?

matthewbauer · 2019-08-28T21:31:34Z

Doesn't this cause the nix executable to be potentially much larger than it should be?

Yeah, but mostly only because those symbols are missing.

$ du /nix/store/gnyc6ywn4zsc61w3xql1b8vr5l55qp0m-nix-2.2.2/bin/nix
1668	/nix/store/gnyc6ywn4zsc61w3xql1b8vr5l55qp0m-nix-2.2.2/bin/nix
$ du /nix/store/7hygziif4mx6bi33ija2iicwjrn1l1dv-nix-2.3pre6629_6587ffd/bin/nix
4548	/nix/store/7hygziif4mx6bi33ija2iicwjrn1l1dv-nix-2.3pre6629_6587ffd/bin/nix

To make things smallers, I've added a flag that just applies it for libstore.a. It doesn't help too much though:

$ du /nix/store/x2xlbs41grl4j60qyb4v3yyki8mc8mhq-nix-2.3pre0_0000000/bin/nix
4312	/nix/store/x2xlbs41grl4j60qyb4v3yyki8mc8mhq-nix-2.3pre0_0000000/bin/nix

Try:

(import ./release.nix {}).build.x86_64-linux.overrideAttrs (old: {
  configureFlags = old.configureFlags or [] ++ [ "--disable-shared" ];
})

$ nix build -f test.nix
$ ./result/bin/nix run nixpkgs.hello

Notice that the binary cache isn't working, because the regStore symbols aren't linked into the executable!

matthewbauer · 2019-08-30T21:11:40Z

Pinging @edolstra @grahamc once more. This does increase static executable sizes, but is also necessary to use any of the store implementations. Without it, static Nix is unusable for anything like the binary cache (because HttpBinaryCacheStore class is missing).

matthewbauer · 2019-09-04T16:47:42Z

I also should note this just effects --disable-shared builds.

edolstra · 2019-09-11T16:11:05Z

--whole-archive seems like a sledgehammer approach. Surely there must be a better way to force variables with constructors to be included. https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes mentions a used attribute: "This attribute, attached to a variable with static storage, means that the variable must be emitted even if it appears that the variable is not referenced." Maybe that does the trick?

matthewbauer · 2019-09-11T17:55:50Z

--whole-archive seems like a sledgehammer approach. Surely there must be a better way to force variables with constructors to be included. https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes mentions a used attribute: "This attribute, attached to a variable with static storage, means that the variable must be emitted even if it appears that the variable is not referenced." Maybe that does the trick?

I don't think there is. __attribute__((used)) is a compiler concept and is never passed to the linker from what I can tell. I have a test branch to try this here:

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:

https://stackoverflow.com/questions/1202494/why-doesnt-attribute-constructor-work-in-a-static-library

Two alternatives are possible:

Rework the build scripts to get rid of "libnixstore.a" and compile libnixstore sources directly with the nix binary.
Use ifdef to manually run the regStore constructors when we know we are linking to static libnixstore.a.

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 --whole-archive is the best solution here.

saschagrunert · 2019-10-30T20:05:21Z

Do you think that we can go forward with this PR somehow? I would love to see a recent static nix version. :)

matthewbauer · 2019-10-30T21:22:37Z

At NixCon, @edolstra wanted to see if #3154 would fix this. Building it right now.

matthewbauer · 2019-11-01T15:26:26Z

@edolstra Unfortunately #3154 doesn't fix this. We still need --whole-archive here.

tomberek · 2019-11-25T04:29:41Z

Would it make sense to proceed for now with --whole-archive as a sledgehammer for the static build, then adopt whatever might be better in the future?

PkmX · 2020-04-10T17:42:05Z

I also hit this issue with a statically-linked nix. This also affects libexpr where some primops (e.g. fetchGit) would not get pulled in unless --whole-archive is used.

PkmX · 2020-04-17T09:08:42Z

For reference, libfetchers in the latest master also needs the --whole-archive flag.

Mic92 · 2020-04-17T15:04:52Z

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.

Mic92 · 2020-04-17T15:47:11Z

All classes that make use of static initializers:

RegisterPrimOp, RegisterCommand, RegisterLegacyCommand, RegisterStoreImplementation and GlobalConfig::Register

I think as long as each object file contains at least one exported symbol that is linked, their static initializers are loaded as well.

Mic92 · 2020-04-17T16:41:24Z

I opened #3511 as an alternative fix.

matthewbauer · 2020-04-17T18:23:05Z

Is store-api.hh supposed to support store implementations that are not part of the nix code base?

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)

Mic92 · 2020-05-07T10:32:32Z

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.

edolstra · 2020-05-07T10:49:41Z

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.

Mic92 · 2020-05-07T11:11:02Z

This issue is not fixable on a language level without refactorings like #3511.
The linker, who does not care about the language, won't include object files into the binary unless at least one symbol from it is used unless --whole-archive is set. I also don't see any downside from using this options. All object files build are eventually used in the application anyway so it does not make the build less efficient or bigger.

shlevy · 2020-05-07T11:18:57Z

@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.

matthewbauer · 2020-05-07T17:33:09Z

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.

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 --gc-sections set. If you don't get the observed bug with --gc-sections, I would say this is a linker bug. Otherwise, I think it's a Nix bug.

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 -c) - gcc might be smart enough to know how to handle this edge case. This kind of screws up the separation between the Nix libraries but may be useful if it actually avoid the problem.

Kha · 2020-05-07T18:23:56Z

This behavior is specified, or at least allowed, by the C++ standard (if I'm reading it correctly):

If no variable or function is odr-used from a given translation unit, the non-local variables defined in that translation unit may never be initialized (this models the behavior of an on-demand dynamic library).
https://en.cppreference.com/w/cpp/language/initialization#Non-local_variables

That is, if you do not use a source file at all, the compiler is free to forego initialization.

matthewbauer · 2020-06-10T04:41:06Z

Closing in favor of #2698 since i think that is a better approach. It makes static and dynamic linking closer to each other.

xaverdh · 2020-06-24T09:52:43Z

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 ?

matthewbauer changed the title ~~Add --whole-archive to include all store implementations~~ Add --whole-archive to include all store implementations (when BUILD_SHARD_LIBS=false) Mar 2, 2019

matthewbauer changed the title ~~Add --whole-archive to include all store implementations (when BUILD_SHARD_LIBS=false)~~ Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) Mar 2, 2019

matthewbauer mentioned this pull request Mar 28, 2019

Make nix/unpack-channel.nix a builtin builder #2748

Merged

grahamc reviewed May 17, 2019

View reviewed changes

matthewbauer force-pushed the fix-static-linking branch from e521c09 to 6587ffd Compare July 1, 2019 02:11

matthewbauer mentioned this pull request Sep 29, 2019

nix: allow static compilation NixOS/nixpkgs#70024

Merged

3 tasks

This comment has been minimized.

Sign in to view

matthewbauer mentioned this pull request Jan 26, 2020

Host a static nix build nix-community/nix#3

Closed

PkmX mentioned this pull request Apr 16, 2020

Static initialization order between evalSettings and getHome2 #3507

Open

Mic92 mentioned this pull request Apr 17, 2020

[RFC] libstore: remove static initialisation #3511

Closed

matthewbauer added 3 commits April 17, 2020 13:25

Handle Darwin linker as well

59f93a9

I forgot to verify this on macOS. We need to use -Wl,-force_load here.

Make --whole-archive optional for each library

d81e2af

Set _WHOLE_ARCHIVE for libexpr, libfetchers

c4794a6

These also need it: - NixOS#2698 (comment) - NixOS#2698 (comment)

matthewbauer force-pushed the fix-static-linking branch from cc530c2 to c4794a6 Compare April 17, 2020 18:26

matthewbauer closed this Jun 10, 2020

matthewbauer mentioned this pull request Jun 10, 2020

Prelink static libraries into an object file #3677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698

Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698

matthewbauer commented Feb 24, 2019 •

edited

grahamc May 17, 2019

matthewbauer May 17, 2019

matthewbauer commented Aug 28, 2019

edolstra commented Aug 28, 2019

matthewbauer commented Aug 28, 2019

matthewbauer commented Aug 30, 2019

matthewbauer commented Sep 4, 2019

edolstra commented Sep 11, 2019

matthewbauer commented Sep 11, 2019

saschagrunert commented Oct 30, 2019

matthewbauer commented Oct 30, 2019

matthewbauer commented Nov 1, 2019

tomberek commented Nov 25, 2019

This comment has been minimized.

PkmX commented Apr 10, 2020 •

edited

PkmX commented Apr 17, 2020 •

edited

Mic92 commented Apr 17, 2020 •

edited

Mic92 commented Apr 17, 2020 •

edited

Mic92 commented Apr 17, 2020

matthewbauer commented Apr 17, 2020

Mic92 commented May 7, 2020 •

edited

edolstra commented May 7, 2020

Mic92 commented May 7, 2020 •

edited

shlevy commented May 7, 2020

matthewbauer commented May 7, 2020

Kha commented May 7, 2020

matthewbauer commented Jun 10, 2020

xaverdh commented Jun 24, 2020

Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698

Add --whole-archive to include all store implementations (when BUILD_SHARED_LIBS=false) #2698

Conversation

matthewbauer commented Feb 24, 2019 • edited

grahamc May 17, 2019

Choose a reason for hiding this comment

matthewbauer May 17, 2019

Choose a reason for hiding this comment

matthewbauer commented Aug 28, 2019

edolstra commented Aug 28, 2019

matthewbauer commented Aug 28, 2019

matthewbauer commented Aug 30, 2019

matthewbauer commented Sep 4, 2019

edolstra commented Sep 11, 2019

matthewbauer commented Sep 11, 2019

saschagrunert commented Oct 30, 2019

matthewbauer commented Oct 30, 2019

matthewbauer commented Nov 1, 2019

tomberek commented Nov 25, 2019

This comment has been minimized.

PkmX commented Apr 10, 2020 • edited

PkmX commented Apr 17, 2020 • edited

Mic92 commented Apr 17, 2020 • edited

Mic92 commented Apr 17, 2020 • edited

Mic92 commented Apr 17, 2020

matthewbauer commented Apr 17, 2020

Mic92 commented May 7, 2020 • edited

edolstra commented May 7, 2020

Mic92 commented May 7, 2020 • edited

shlevy commented May 7, 2020

matthewbauer commented May 7, 2020

Kha commented May 7, 2020

matthewbauer commented Jun 10, 2020

xaverdh commented Jun 24, 2020

matthewbauer commented Feb 24, 2019 •

edited

PkmX commented Apr 10, 2020 •

edited

PkmX commented Apr 17, 2020 •

edited

Mic92 commented Apr 17, 2020 •

edited

Mic92 commented Apr 17, 2020 •

edited

Mic92 commented May 7, 2020 •

edited

Mic92 commented May 7, 2020 •

edited