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.

Sign up for GitHub

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

Jump to bottom

autoPatchelfHook: Use alternative interpreters in multilib envs #51588

Closed
wants to merge 1 commit into from
Closed

autoPatchelfHook: Use alternative interpreters in multilib envs #51588

wants to merge 1 commit into from

Conversation

tadfisher
Copy link
Contributor

@tadfisher tadfisher commented Dec 5, 2018 •
edited

Motivation for this change

autoPatchelfHook correctly restricts the architecture of libraries patched into RPATH, but it does not check that the correct dynamic linker is used for INTERP; instead, it always applies the host's native dynamic linker. This can result in 32-bit binaries using ld-linux-x86-64.so.2 on x86-64 systems, for example.

This change passes in both the native and 32-bit linkers for the host platform (note that these can be the same depending on the platform). autoPatchelfFile then applies the 32-bit linker for ELF32 files, and the native linker otherwise.

This change searches for interpreters in $NIX_CC/nix-support that support the architecture of the patched binary. When building in a multilib stdenv, only a matching interpreter will be patched, otherwise the build fails (with an appropriate error message).

Mixed 32- and 64-bit binaries can now be patched automatically by autoPatchelfHook with no manual fixup needed. I tested existing packages (which are not mixed) using nox-review on an x86-64 machine, and also by building nixpkgs.pkgsi686Linux.<pkg> for the smattering of packages using this hook that build on i686 natively.

cc @aszlig

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

tadfisher
tadfisher commented Dec 5, 2018
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
substitutions = rec {
ld = "${stdenv.cc}/nix-support/dynamic-linker";
ld32 = if stdenv.hostPlatform.is32bit then ld
else "${stdenv_32bit.cc}/nix-support/dynamic-linker-m32";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A shortcut in stdenv.cc.bintools (like the existing dynamicLinker) would remove the reliance on this output, as it's probably only supported for GCC libc.

aszlig
aszlig requested changes Dec 6, 2018
View reviewed changes
Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, this is something I missed when writing the implementation in the first place.

However, as pointed out in my comments, I don't like the approach of hardcoding of the various stdenv variants into the shell hook as it makes certain assumptions on the architectures in use.

pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
pkgs/development/libraries/cutelyst/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/cutelyst/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
@tadfisher tadfisher changed the title autoPatchelfHook: Use multilib interpreter for ELF32 binaries autoPatchelfHook: Use alternative interpreters in multilib envs Dec 6, 2018
@tadfisher
Copy link
Contributor Author

@aszlig This has been redone with your suggestions in mind; namely, searching through interpreters in $NIX_CC/nix-support to find one with a matching architecture.

Thank you for the help!

@mmahut
Copy link
Member

mmahut commented Aug 12, 2019

Are there any updates on this pull request, please?

@tadfisher
Copy link
Contributor Author

@mmahut I would still like to see this merged if possible, as I continue to use this patch in my personal projects.

@aszlig Are you available to review this?

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/41

@tadfisher tadfisher mentioned this pull request Oct 8, 2019
Closed
4 tasks
@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
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@tadfisher tadfisher closed this Jan 7, 2021
@aszlig
Copy link
Member

aszlig commented Jan 8, 2021

@tadfisher: Damn, I'm sorry for not getting back to you. Was swamped in notifications and only recently (a few months ago) overhauled the notification settings. Apparently the branch had merge conflicts, so if you still want to get this in, can you please rebase and reopen?

(and yes, this time I should get the notifications, sorry again...)

@impl impl mentioned this pull request Sep 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig requested changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees
No one assigned
Labels
2.status: merge conflict 8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tadfisher @mmahut @nixos-discourse @aszlig @ryantm @GrahamcOfBorg