pass correct pie args to gcc linker #48076
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. |
|
Can we pass the |
|
no, the -no-pie flag will cause older versions of gcc to error out because it doesn't recognize the flag. |
arazabishov
added
the
S-waiting-on-review
|
Thanks for the PR @canarysnort01! Would it be possible to avoid running |
|
Yes, this would run the version check on all platforms on all executables when the link step is configured to use gcc. The benefit here is we know for sure how gcc is configured and we're doing the right thing. An alternative approach would be:
The benefit to this approach would be the user only has to pay for the extra linker invocation when it is actually necessary, and would get used less and less as the user base upgrades gcc. |
|
When was |
|
According to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=867853 around gcc 5.3, so a couple years, although it doesn't appear to have been documented until 6.1. The gcc 6.3 in debian stable (stretch) accepts -no-pie, but the gcc in debian oldstable (jessie) (I think 4.8) does not. |
|
Ok thanks for the info! I think that sounds like a reasonable approach in that case. I believe by default all Rust executables are pie and so it won't hit the "slow path" anyway, but in those cases it does then a gcc upgrade should do it! |
src/librustc_trans/back/link.rs
Outdated
| // is safe because if the linker doesn't support -no-pie then it should not | ||
| // default to linking executables as pie. Different versions of gcc seem to | ||
| // use different quotes in the error message so don't check for them. | ||
| if out.contains("unrecognized command line option") && out.contains("-no-pie") { |
There was a problem hiding this comment.
To help prevent an accidental infinite loop, could this also have a clause that the cmd has an argument of -no-pie in it?
|
@bors: r+ |
|
📌 Commit bc82fec has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- Failed on macOS ( |
bors
added
S-waiting-on-author
src/librustc_trans/back/link.rs
Outdated
| // is safe because if the linker doesn't support -no-pie then it should not | ||
| // default to linking executables as pie. Different versions of gcc seem to | ||
| // use different quotes in the error message so don't check for them. | ||
| if out.contains("unrecognized command line option") && |
There was a problem hiding this comment.
This should check for "error: unknown argument" as well.
|
Since this probably isn't applicable to OSX as well, could this logic only get triggered on unix targets? |
|
@alexcrichton IIUC you want to check if the target is an OSX target and, if so, never issue the -no-pie argument at all? It looks like clang might follow in gcc's footsteps in this respect, here's an open issue to do just that: https://bugs.llvm.org/show_bug.cgi?id=13410 Also, it looks like I'm concerned if we add this exception and osx upgrades/patches their clang to default to pie it will break. |
|
Is the |
|
Ah I think see where you are coming from now. I was thinking the build failure from @kennytm was from the test suite like in #47037 which I would expect to hit this code path, but iiuc now all builds on OSX failed. (I don't have a mac to test with). Which makes sense now that I see that position-independent-executables isn't set in OSX's target or the mac base target. Which leads back to does pie mean anything on OSX... hmm good question. |
|
I don't have an answer yet, but I think one option to address this would be to make position-independent-executables have three possible values: yes, no, default.
|
|
Invoking the linker tends to be a fickle beast so it's often best to leave it as is unless we need to change it, so perhaps a flag could be added to only pass the flag when |
|
That seems like a reasonable solution. |
When linking with gcc, run gcc -v to see if --enable-default-pie is compiled in. If it is, pass -no-pie when necessary to disable pie. Otherwise, pass -pie when necessary to enable it. Fixes rust-lang#48032 and fixes rust-lang#35061
Recent versions of gcc default to creating a position independent executable and must be explicitly told not to with the -no-pie argument. Old versions of gcc don't understand -no-pie and will throw an error. Check for that case and retry without -no-pie. This is safe because these old versions of gcc should never default to creating a position independent executable.
|
Should I update this PR description to the current solution? Is that what gets put in the merge commit description? |
canarysnort01
force-pushed
the
fix_pie
branch
from
d5d4edf
to
ab9cae1
Compare
|
@alexcrichton this pr will conflict with #48125 |
kennytm
added
S-waiting-on-review
|
📌 Commit ab9cae1 has been approved by |
bors
added
S-waiting-on-bors
kennytm
added
S-waiting-on-bors
pass correct pie args to gcc linker When linking with gcc, run gcc -v to see if --enable-default-pie is compiled in. If it is, pass -no-pie when necessary to disable pie. Otherwise, pass -pie when necessary to enable it. Fixes rust-lang#48032 and fixes rust-lang#35061
When linking with gcc, run gcc -v to see if --enable-default-pie is
compiled in. If it is, pass -no-pie when necessary to disable pie.
Otherwise, pass -pie when necessary to enable it.
Fixes #48032 and fixes #35061