binutils: Enable new dtags in a way that works with binutils 2.30.

[Rotaerk]

Motivation for this change

In 3027bca, binutils was upgraded from 2.28.1 to 2.30. However, in 2.30, the ldmain.c file within binutils, which the nixpkgs new-dtags.patch file is meant to modify, was changed in such a way that the patch no longer works. As a result, the new dtags are not actually enabled, and binaries are built with RPATH set instead of RUNPATH, thereby preventing LD_LIBRARY_PATH from overriding this built-in path. This change corrects this. The patch file is no longer necessary because binutils's ldmain.c now sets link_info.new_dtags based on the configuration flags.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Fits CONTRIBUTING.md.

---

[xeji]

cc @Ericson2314

[dtzWill]

I haven't looked into this but

1. Wow! Awesome find and thanks for reporting! Really unfortunate that we missed this! I wonder how much this will explain/break? o___O.

2. Let's put together a simple test (maybe even installCheck?) that ensures this doesn't happen again.

[Ericson2314]

Great work! Agreed re test.

[Rotaerk]

Something I didn't really do, but should probably be done, is to make sure the other patches aren't also broken. I wonder if checking patch validity is something that can be generalized in nixpkgs.

[dezgeg]

I'm confused. I wrote a test:

with import ./. {};

# This tests that libraries listed in LD_LIBRARY_PATH take precedence over those listed in RPATH.
let
  libhello = stdenv.mkDerivation {
    name = "libhello";

    code = ''
      const char* getGreeting() { return "Hello, world!"; }
    '';

    unpackPhase = ''
      echo "$code" > libhello.c
    '';

    installPhase = ''
      mkdir -p $out/lib
      $CC -c -fpic libhello.c
      $CC -shared libhello.o -o $out/lib/libhello.so
    '';
  };

  libgoodbye = libhello.overrideAttrs (_: {
    name = "libgoodbye";
    code = ''
      const char* getGreeting() { return "Goodbye, world!"; }
    '';
  });

  testProgram = stdenv.mkDerivation {
    name = "hello-test";

    buildInputs = [ libhello ];

    code = ''
      #include <stdio.h>

      extern const char* getGreeting(void);

      int main() {
        puts(getGreeting());
      }
    '';

    unpackPhase = ''
      echo "$code" > hello-test.c
    '';

    installPhase = ''
      mkdir -p $out/bin
      $CC -c hello-test.c
      $CC hello-test.o -lhello -o $out/bin/hello-test
    '';
  };

in runCommand "test-LD_LIBRARY_PATH" { nativeBuildInputs = [ testProgram ]; } ''
  [ "$(hello-test | tee /dev/stderr)" = "Hello, world!" ]
  [ "$(LD_LIBRARY_PATH=${libgoodbye}/lib hello-test | tee /dev/stderr)" = "Goodbye, world!" ]
  touch $out
''

and it seems to work as expected even without this patch.

[Rotaerk]

If you readelf -d the resulting hello-test binary, does it show RPATH or RUNPATH? And what path does it contain?

[dezgeg]

In the final binary it is:

 0x000000000000001d (RUNPATH)            Library runpath: [/nix/store/ahv413v2qiasgxpz7kihar9nrvbpv9k6-libhello/lib:/nix/store/l0adcz8y05vajfi483a7qv31i03rspqq-glibc-2.27/lib]

But right after compiling it is:

 0x000000000000000f (RPATH)              Library rpath: [/nix/store/327cpbvca27xk9hpzscq8qzmk9nq6di4-hello-test/lib64:/nix/store/327cpbvca27xk9hpzscq8qzmk9nq6di4-hello-test/lib:/nix/store/ahv413v2qiasgxpz7kihar9nrvbpv9k6-libhello/lib:/nix/store/l0adcz8y05vajfi483a7qv31i03rspqq-glibc-2.27/lib:/nix/store/i5znwsdi9yybfydd1kbdrsq5s4phjm93-gcc-7.3.0-lib/lib]

I suppose the patchelf --shrink-rpath done in fixupPhase changes it to RUNPATH from RPATH. I guess it also explains why this issue hasn't really been noticed before. (I think without LD_LIBRARY_PATH working all OpenGL apps would stop working on proprietary drivers, which would obviously be noticed quickly).

[Rotaerk]

Interesting. That could explain the inconsistency I was seeing. Particularly, if I built my http://github.com/rotaerk/vulkanTest project with nix-build, the resulting binary contained RUNPATH, and ran fine. But if I built it using cabal build from within a nix-shell, the resulting binary contained RPATH, and threw some libGL: undefined symbol error an runtime.

This ultimately was made to work without my binutils patch (leaving the RPATH in place) just by switching NixOS to nixos-unstable-small. I believe this is because it brought the OpenGL driver provided by NixOS up to a version that was in sync with the one in my targeted nixpkgs. But the issue with binutils still stands, regardless, and was a convenient accidental discovery.

[dezgeg]

Yes, makes sense. I double-checked patchelf documentation and it indeed states:

--force-rpath
    Forces the use of the obsolete DT_RPATH in the file instead of
    DT_RUNPATH. By default DT_RPATH is converted to DT_RUNPATH. 

The patch looks fine but it could use a commit message (including this new patchelf discovery).

[Rotaerk]

How about this for a commit message? (I'll change the commit after it's looked over.)

binutils: Enable new dtags in a way that works with binutils 2.30.

In 3027bca, binutils was upgraded from 2.28.1 to 2.30. However, in 2.30, the ldmain.c file
within binutils, which the nixpkgs new-dtags.patch file is meant to modify, was changed in
such a way that the patch no longer works. As a result, the new dtags are not actually
enabled, and binaries are built with RPATH set instead of RUNPATH, thereby preventing
LD_LIBRARY_PATH from overriding this built-in path. This change corrects this. The patch
file is no longer necessary because binutils's ldmain.c now sets link_info.new_dtags based
on the configuration flags.

This was probably not noticed immediately because, when the derivation is built with
nix-build, the fixupPhase runs patchelf --shrink-rpath.  patchelf converts any RPATH in the
binary into RUNPATH (unless --force-rpath is specified).  Of course, if the binary is built
without nix-build (such as in a nix-shell), this never occurs, and any RPATH in the binary
is left in place.

[dezgeg]

Yes, sounds good.

[dezgeg]

#44301 adds a test.