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

binutils-unwrapped: install gcc LTO plugin #188544

Closed
wants to merge 3 commits into from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Aug 27, 2022

Reviewers' note: I did a very hacky condition of gcc and binutils compatibility. I think it's slightly incorrect for cross case. Please have a close look at it.

Before the change binutils was not able to handle LTO bytecode without
explicit plugin being passed:

$ touch a.c && gcc -flto -fPIC -c a.c -o a.o && ar cr a.a a.o
ar: a.o: plugin needed to handle lto object

After the change binutils can now handle LTO bytecode and build valid
indices:

$ touch a.c && gcc -flto -fPIC -c a.c -o a.o && ar cr a.a a.o
<ok>

The change fixes LTO build of sway:

$ nix build --impure --expr 'with import ./. {}; sway-unwrapped.overrideAttrs (oa: { NIX_CFLAGS_COMPILE = "-flto=auto"; NIX_CFLAGS_LINK = "-flto=auto"; })
Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@trofi
Copy link
Contributor Author

trofi commented Aug 28, 2022

Fixed bootstrapTools (and cross-bootstrapTools) building by removing LTO plugin link as otherwise it leaks out reference to boot gcc.

@lovesegfault
Copy link
Member

I think these changes are correct, but I'd love for @Ericson2314 to take a look before we merge it.

Especially the cross implications of this.

@Ericson2314
Copy link
Member

We shouldn't have binutills depend on GCC that is backwards bootstrapping. We have have the wrapper or a new derivation instead "mix in" this plugin.

@trofi
Copy link
Contributor Author

trofi commented Aug 31, 2022

We shouldn't have binutills depend on GCC that is backwards bootstrapping. We have have the wrapper or a new derivation instead "mix in" this plugin.

That makes sense.

  1. We'll need to patch binutils a bit to allow that kind of late injection.
  2. We'll need to make a bunch of binaries to apply the override. Binaries are (non-exhaustive): nm, strip, ld, ranlib, objcopy and probably others that have a chance to load linker plugin to understand LTO bytecode.

To avoid the need of need a bunch of makeWrapper's we can inject the path via presence of a file in /nix-support/. Or somehow add a PATH of the wrapper as well to the search path.

Currently binutils hardcodes plugin search path to be relative to it's lib and bin/../lib folder: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/plugin.c;h=c69fbd7b568ce49d63aa949d1a866c602a9f68f4;hb=HEAD#l487

static void
build_plugin_list (bfd *abfd)
{
  /* The intent was to search ${libdir}/bfd-plugins for plugins, but
     unfortunately the original implementation wasn't precisely that
     when configuring binutils using --libdir.  Search in the proper
     path first, then the old one for backwards compatibility.  */
  static const char *path[]
     = { LIBDIR "/bfd-plugins", BINDIR "/../lib/bfd-plugins" };
...

@Ericson2314
Copy link
Member

Let's make an env var and try to upstream it?

Before the change binutils was not able to handle LTO bytecode without
explicit plugin being passed:

    $ touch a.c && gcc -flto -fPIC -c a.c -o a.o && ar cr a.a a.o
    ar: a.o: plugin needed to handle lto object

After the change binutils can now handle LTO bytecode and build valid
indices:

    $ touch a.c && gcc -flto -fPIC -c a.c -o a.o && ar cr a.a a.o
    <ok>

The change fixes LTO build of sway:

    $ nix build --impure --expr 'with import ./. {}; sway-unwrapped.overrideAttrs (oa: { NIX_CFLAGS_COMPILE = "-flto=auto"; NIX_CFLAGS_LINK = "-flto=auto"; })
Note: it's an incompletel wrap: we only populate plugin link for
`ld`, `strip` (existing wrappers) and `ar` (new wrapper).

If we go that route we'll need to wrap most binaries for GNU binutils
as almost all of them can detect plugin format and act on it accordingly:
`size`, `nm`, `ranlib`, `addr2line`.
@github-actions github-actions bot removed the 6.topic: stdenv Standard environment label Sep 3, 2022
@trofi
Copy link
Contributor Author

trofi commented Sep 3, 2022

@Ericson2314 I switched to runtime detection of a plugin and switched to plugin injection at binutils-wrapper site. PTAL.

CAVEAT: I created only a subset of binutils wrappers: ld, ar and strip. If that is the way to go we will need to wrap virtually every binary: ranlib, nm, addr2line and probably a ten more.

I find the result quite messy :)

  1. Is it laid out you would expect? Or I misinterpreted it? If it's ok I'll send the patch upstream and wrap the rest of binutils.
  2. Can you elaborate a bit why you are against injecting the plugin at binutils build time?

@Ericson2314
Copy link
Member

@trofi OK this is better just a few things things left:

  1. The wrapper should not get gcc out of stdenv; that is the stdenv used to build the wrapper itself which is the completely wrong one.
  2. The wrapper shouldn't know about GCC at all, it should just take a list of paths. Code elsewhere is responsible for putting
  3. The env var shouldn't be a single path, but a :-separated list of paths. I think there is more precedent for that.

Can you elaborate a bit why you are against injecting the plugin at binutils build time?

The ideal bootstrapping is:

  1. Build Binutils once with boostrap compiler
  2. Build GCC once with bootstrap compiler

If bintuils depends on GCC for the plugin, however, we need to build it twice:

  1. Build Binutils once with boostrap compiler
  2. Build GCC once with bootstrap compiler
  3. Build Binutils again, with either compiler, to get plugin.

This is wasteful.

The only reason you didn't notice this is because the current convoluted native bootstrapping obscures it. But I would want to get LTO support for cross native alike, so I rather not rely on that tech debt serendipitously helping us :).

@trofi
Copy link
Contributor Author

trofi commented Sep 12, 2022

@trofi OK this is better just a few things things left:

1. The wrapper should not get gcc out of `stdenv`; that is the stdenv used to build the wrapper itself which is the completely wrong one.

Aha, I'll spend some time understanding what is the correct gcc passed to wrapper.

2. The wrapper shouldn't know about GCC at all, it should just take a list of paths. Code elsewhere is responsible for putting

Aha, makes sense. One option would be to put it to pkgs/top-level/all-packages.nix with possible complications of passing a different/empty value during bootstrap.

3. The env var shouldn't be a single path, but a `:`-separated list of paths. I think there is more precedent for that.

That makes sense. Given that binutils is portable across windows / linux specific : separator might be a problem. I'll look around to see closest match. Maybe it's fine to make plugin loading UNIX-specific.

I'll turn it into a WIP ntil I update the patchset.

Can you elaborate a bit why you are against injecting the plugin at binutils build time?

The ideal bootstrapping is:

1. Build Binutils once with boostrap compiler

2. Build GCC once with bootstrap compiler

For my understanding: do you describe one step of bootstrap or full bootstrap? If it's a full bootstrap we will get binaries built by (prebuilt) bootstrap compiler. Which might be problematic if nixpkgs compiler has some generator bugs fixed compared to bootstrap compiler.

If bintuils depends on GCC for the plugin, however, we need to build it twice:

1. Build Binutils once with boostrap compiler

2. Build GCC once with bootstrap compiler

3. Build Binutils again, with either compiler, to get plugin.

This is wasteful.

The only reason you didn't notice this is because the current convoluted native bootstrapping obscures it. But I would want to get LTO support for cross native alike, so I rather not rely on that tech debt serendipitously helping us :).

Aha, makes sense.

I guess it depends on the goals of nixpkgs's native bootstrap. What are they? I could think of a few:

  1. Do we do minimal amount of work to get a working compiler? In this case just unpacking bootstrap compiler might be good enough.

  2. Do we want to disentangle from bootstrap tools as a direct runtime dependency? Then building binutils and gcc once(-ish, if we ignore libc) each that might be OK.

  3. Do we want to disentangle from code generated by bootstrap compiler in final binaries? Then we need to build gcc and binutils at least twice: with bootstrap compiler and then with built compiler. For example I think our current nixpkgs's glibc is built by a bootstrap compiler (which is observable). I find it amusing and fragile given that we don't refresh bootstrap binaries that frequently (once in a few NixOS releases!).

Current nixpkgs's bootstrap solves [1.] and [2.] but not [3.]. Is it intentional? What does nixpkgs try to achieve?

Also fun fact: current bootstrap builds binutils 3 times:

  1. first is built by bootstrap compiler and is linked against bootstrap glibc (in stage1)
  2. second is built by freshy built gcc, bootstrap coreutils (stage4)
  3. third is built by fresh coreutils (final stage)
$ nix-store --query --graph $(nix-instantiate -A stdenv) |& grep -P -- '->.*binutils-2.38.drv' | awk '{print $3}' | sort -u
"3c0fv5lsx1plbwqzh0ni4f2b4952853c-binutils-2.38.drv"
"g6a13lf1pql6i9kqqyinhhi9wsf7m2h5-binutils-2.38.drv"
"kxx5c5d03pm59c1nvh7c14f80lb93hfm-binutils-2.38.drv"

@trofi trofi marked this pull request as draft September 12, 2022 20:15
@Ericson2314
Copy link
Member

@trofi So my long term goal is to make all bootstrapping, native or cross, build the same way. And also to minimize dependencies / avoid extra rebuild steps. In that regard, just on basic intuition, compilers (may) need linkers, but linkers shouldn't assume the existence of any specific compiler because they can be used with many. In fact, at build time, neither compilers or linkers should be aware of each other because tool chains can be put together after the fact in many different ways.

Basically, we've fought vary hard to separate concerns and untangle things, and this gives us the flexible to solve bootstrapping problems and other problems in more and nicer ways, and I want to keep that separation of concerns and flexibility.

@Ericson2314
Copy link
Member

As to search path, yes maybe a flag would be better. I guess keep it a single env var for now, but please do email the mailing list so they can bikeshed what they want and we aren't left guessing.

@trofi
Copy link
Contributor Author

trofi commented Sep 15, 2022

As to search path, yes maybe a flag would be better. I guess keep it a single env var for now, but please do email the mailing list so they can bikeshed what they want and we aren't left guessing.

To email something material I need more precise semantics than "a flag, or a path, or a list of paths". There already is a --plugin flag that most binutils tools accept. I'm still unclear what semantics list of paths would have or list of plugin has. My guess it's cumulative loading of all the known plugins.

I'll abandon this PR until I get a bit better understanding what our bootstrap is achieving to see how LTO fits into it.

@trofi trofi closed this Sep 15, 2022
@trofi trofi deleted the binutils-use-linker-plugin branch September 15, 2022 22:03
@Ericson2314
Copy link
Member

Aw, I was excited to see this happen. To me the semantics of search path should be just like -L vs -l. Where is --plugin documented? I having trouble finding it.

@trofi
Copy link
Contributor Author

trofi commented Sep 17, 2022

$ LANG=C nm --help |& fgrep plugin
      --plugin NAME      Load the specified plugin

$ LANG=C ar --help |& fgrep plugin
Usage: ar [emulation options] [-]{dmpqrstx}[abcDfilMNoOPsSTuvV] [--plugin <name>] [member-name] [count] archive-file file...
  --plugin <p> - load the specified plugin

man ar seems to also contain the details:

       --plugin name
           The optional command-line switch --plugin name causes ar to load the plugin called name which adds support for more file formats, including object files with link-time optimization information.

           This option is only available if the toolchain has been built with plugin support enabled.

           If --plugin is not provided, but plugin support has been enabled then ar iterates over the files in ${libdir}/bfd-plugins in alphabetic order and the first plugin that claims the object in
           question is used.

           Please note that this plugin search directory is not the one used by ld's -plugin option.  In order to make ar use the  linker plugin it must be copied into the ${libdir}/bfd-plugins directory.
           For GCC based compilations the linker plugin is called liblto_plugin.so.0.0.0.  For Clang based compilations it is called LLVMgold.so.  The GCC plugin is always backwards compatible with earlier
           versions, so it is sufficient to just copy the newest one.

I'm not sure how plugins could follow -L. I see -L / -l as flags for targets. While plugin loading is for build compiler.

Given that plugin is loaded into address space of the compiler there are extra constraints how compatible they are: for example I don't expect gcc plugin built against musl libc runtime to work when loaded into ld built against glibc.

For fancier cases like ld.gold (which is itself linked against libstdc++) we also need to make sure gcc plugin uses identical (or at least ABI-compatible) libstdc++.

The indirection of binutils-wrapper -> binutils -> gcc-wrapper -> gcc does not look trivial to maintain these tight invariants.

@lovesegfault
Copy link
Member

@trofi Given all your recent work on bootstrap, and the improvements amjoseph landed, how do you feel about giving this another shot?

Happy to chat on Matrix about it, as soon as I manage to log into my account :^)

@trofi
Copy link
Contributor Author

trofi commented Apr 27, 2023

Yeah, I keep the tab open to get it working eventually.

Currently stdenv is in a bit of flux and will require more cleanups to fix things like gccgo (#209870 (comment)) and other one-off packages. When those settle down I'll try to plumb the plugin in a less hacky way.

@trofi
Copy link
Contributor Author

trofi commented Jul 15, 2023

I'm dropping the ball on that.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-correctly-use-ld-nm-ar-ranlib-with-lto-in-nix-develop-shell/33220/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants