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

[WIP] Set absolute sonames #45105

Closed
wants to merge 3 commits into from
Closed

Conversation

lheckemann
Copy link
Member

Motivation for this change

#24844

Most stuff I've tried building with this works. However, nixos won't build with this yet because the extra-tools initrd stuff doesn't build successfully. I'm getting the… interesting error message

Inconsistency detected by ld.so: dl-version.c: 205: _dl_check_map_versions: Assertion `needed != NULL' failed!

when it tries to test the udevadm in extra-utils. Help would be appreciated!

Also, thanks very much to @grahamc for having made the aarch64 community builder available. It's been invaluable for testing this, what with all the gcc rebuilds!

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.

@@ -0,0 +1,8 @@
[[ -z "$noAbsoluteSoname" ]] && fixupOutputHooks+=(_doAbsoluteSoname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this option also in doc/stdenv.xml?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@lheckemann
Copy link
Member Author

This should also only run on linux, I just realised. Not sure how exactly to do this though — it should probably be in pkgs/stdenv/linux somewhere rather than pkgs/stdenv/generic/default.nix and guarded by isLinux?

@Ericson2314
Copy link
Member

How close does this get us to being able to remove the --rpath stuff from cc-wrapper?

@lheckemann
Copy link
Member Author

lheckemann commented Aug 16, 2018 via email

@Mic92
Copy link
Member

Mic92 commented Aug 16, 2018

I suppose we will uncover bugs, where we implicitly covered dlopen with our rpath? In any case we would also be closer to the macOS port so we have the same behavior on both platforms, which is a good thing.

@dezgeg
Copy link
Contributor

dezgeg commented Aug 16, 2018

I presume this breaks LD_LIBRARY_PATH?

IIRC at some point I tried a similar thing which instead of making the DT_SONAME directly absolute, it would only patchelf all DT_NEEDED to absolute paths. I think changing DT_SONAME itself makes me slightly more nervous given that AFAICT DT_SONAME is sometimes used as some sort of 'cookie' (e.g. if you try to load both /nix/store/foo/libsomething.so and /nix/store/bar/libsomething.so, if the both have the same DT_SONAME only one of them will be loaded).

But, that solution breaks LD_LIBRARY_PATH just as well. So nowadays I think the easiest way to solve the O(n^2) lookup problem would be to cache the contents of directories in RPATH in the dynamic linker.

@Mic92
Copy link
Member

Mic92 commented Aug 17, 2018

When a library was found in the dependency closure and its soname is replaced by an absolute path in the binary, why would we still need LD_LIBRARY_PATH for the same library?

@lheckemann
Copy link
Member Author

#24844 (comment)

We can still use LD_PRELOAD when necessary.

@vcunat
Copy link
Member

vcunat commented Sep 1, 2018

Fixed the GitHub UI.

@dtzWill
Copy link
Member

dtzWill commented Feb 4, 2019

This seems to rebase pretty easily, but may need to be updated for non-conflict reasons such as marking other gcc versions noAbsoluteSoname (why do these need special handling? do other compilers or runtimes?)

Sure would be great to have this, it's silly for nix to know precisely what libraries to use but every program does a (depending on how you count, n^2) search to rediscover this information.

Anyway I probably don't need to argue for this to the folks on this PR already anyway haha but just a reminder :D.

@ambrop72
Copy link
Contributor

I'm interested in getting this further. What is the current status? From what I understand it needs to be rebased and there is a problem with the initrd build.

Libraries that may need to be overridden (e.g. libGL for drivers that don't support libglvnd) can still be linked the old way (normal soname and directory in RUNPATH). I also think that the same should be done for libc because having more than one instance of libc loaded supposedly is not supported and does not work.

@lheckemann
Copy link
Member Author

@ambrop72 I've started to think that this isn't actually a good approach, and patching the NEEDED entries may be better. I'm still unsure though.

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@Mic92
Copy link
Member

Mic92 commented Jun 2, 2020

@lheckemann do you still plan to finish this?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@lheckemann
Copy link
Member Author

I definitely still want this to happen — if nobody else beats me to it, I'll have a look at reworking this to patch DT_NEEDED entries At Some Point™.

@Profpatsch
Copy link
Member

Profpatsch commented Jun 9, 2020 via email

@ryantm ryantm marked this pull request as draft October 23, 2020 03:12
@FRidh FRidh removed their request for review November 21, 2020 11:03
@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
@lheckemann
Copy link
Member Author

NixOS/patchelf#357 implements setting absolute NEEDED entries, and seems to be quite promising.

@lheckemann lheckemann closed this Jan 28, 2022
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.

None yet