Keep sysroot rpath entries as absolute.

[staktrace]

Fixes rust-lang/cargo#12746

When generating rpath entries for the final binary, rustc generally
turns the paths into relative paths. This allows for binaries to be
moved around (as long as their dependency libraries get moved as well)
and the binary still runs.

However, if the libraries are part of the rustc sysroot, then it doesn't
make sense to make these rpath entries relative, because the sysroot is
unlikely to be moved with the binary. Instead, we should keep those
rpath entries absolute.

This patch accomplishes this by checking to see if a given library
dependency lives inside the sysroot lib dir, and if it does, keeps the
corresponding rpath entry absolute instead of making it relative.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[cuviper]

How will this affect the distributed binaries of rustc itself? My ~/.rustup/toolchains/... has no relation to the upstream build paths, of course, but rustc's RPATH of $ORIGIN/../lib lets that work fine.

[Mark-Simulacrum]

I often move the whole sysroot around when doing debugging work on the compiler... I don't think this is quite the solution we want. cc @alexcrichton

[staktrace]

How will this affect the distributed binaries of rustc itself? My ~/.rustup/toolchains/... has no
relation to the upstream build paths, of course, but rustc's RPATH of $ORIGIN/../lib lets that
work fine.

Good question, I'm not sure. I had checked readelf -d $(which rustc) on my machine, which didn't have any RPATH at all. That rustc is in .cargo/bin and I didn't realize the one in ~/.rustup/toolchains is different. (It does have an RPATH like you say).

I often move the whole sysroot around when doing debugging work on the compiler... I don't think
this is quite the solution we want.

Do you have any suggestions on what we do want?

[cuviper]

I had checked readelf -d $(which rustc) on my machine, which didn't have any RPATH at all. That rustc is in .cargo/bin and I didn't realize the one in ~/.rustup/toolchains is different.

The one in .cargo/bin is the rustup shim which will invoke the real rustc, considering the current default toolchain and any overrides. You can get the final path with rustup which rustc.

[staktrace]

So I checked the stage2 rustc binary from my build (where I have this patch applied) and it still has the relative RPATH entry. I also note that RPATH != RUNPATH, and this patch changes the RUNPATH entries in generated binaries, while rustc itself has a RPATH entry which presumably is coming from somewhere else. So it seems like this patch won't affect rustc itself, but I would appreciate it if somebody else could verify that.

[staktrace]

See comment starting at

rust/src/bootstrap/bin/rustc.rs

Line 186 in 3ec5a99

// Dealing with rpath here is a little special, so let's go into some

- it looks like even the relative rpath calculation doesn't work for distributing rustc binaries, so it's basically hard-coded. So yeah, my patch shouldn't affect rustc itself. Maybe the same applies to other toolchain binaries?

[alexcrichton]

I've sort of personally been under the impression that -C rpath is deprecated and effectively unusable. I'm not really sure if anyone's still using it, but from past discussions about it I think it's actually impossible for us to do something "correct" here. (in the sense of one flag in the compiler which always "does the right thing")

Is this something that could perhaps be a rustbuild option rather than a rustc change?

[staktrace]

@alexcrichton Assuming rustbuild is used to build rustc (which is what a Google search indicated) that might solve any issues with rustc linkage. However this patch is to fix the rpath in end-user rust apps built by rustc, so I don't see how changing anything in rustbuild would help there. Am I missing something?

[staktrace]

Also for the record #47931 is the root problem I'm trying to solve here, if anybody has suggestions of a quicker way to do that I'm happy to let this drop.

[petrochenkov]

r? @alexcrichton

[pietroalbini]

@alexcrichton can you review this PR?

[alexcrichton]

@staktrace sorry yes, to clarify rustbuild is rustc's own build system, and if this was purely about building the compiler itself I think it'd be best to do this in rustbuild, but it sounds like this is intended to be used instead for user binaries, so rustbuild won't cut it!

Is #47931 still an issue for you? Was that solved with proc-macro2 0.2.3?

[staktrace]

@alexcrichton Once the proc-macro2 solution is fully deployed (new versions of quote and syn on crates.io) that should solve my problem, yes.

[staktrace]

That being said this behavior still feels like a bug and it seems like it would be good to fix.

[alexcrichton]

Oh for sure yeah but in general to me at least it's not clear how we should handle rpath business. We don't have a lot of precedent for target-specific codegen options (like rpath here) and it's not clear how, when invoked through Cargo, rustc would receive an argument for the right rpath anyway.

Would you be ok closing this for now and waiting for the quote/syn release?

[staktrace]

Yeah I guess that's fine by me.