-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve default target options for x86_64-unknown-linux-none #134765
base: master
Are you sure you want to change the base?
Improve default target options for x86_64-unknown-linux-none #134765
Conversation
Without a standard library, we cannot unwind, so it should be panic=abort by default. Additionally, it does not have std because while it is Linux, it cannot use libc, which std uses today for Linux.
These commits modify compiler targets. |
The panic abort looks good to me. For context, it was actually more of an oversight since I already use this cargo config in most/all of my test projects: [unstable]
build-std = ["core", "compiler_builtins", "alloc"]
build-std-features = ["compiler-builtins-mem", "panic_immediate_abort"]
[build]
rustflags = ["-Cpanic=abort"]
target = "x86_64-unknown-linux-none" About std support - we might add it in the future, but that's still up for discussion. If we do end up getting std support for the target, changing the default panicking strategy should be fine. I don't think rust targets make any guarantees about that. For the relocation model, I'm running some tests right now. To be honest, I need to check if there was a specific reason for defaulting to PIE or if I just overlooked it when writing the target. I remember having some interesting discussions about this with the maintainer of rustix and origin, which might have factored into the decision - I'll look back at those conversations. |
This comment has been minimized.
This comment has been minimized.
In general targets are not beholden to such, no. |
After digging deeper, I have some reservations about switching to a static relocation model. One of the primary goals with this target is enabling developers to write both a libc and dynamic linker in rust. This means we need to support building shared objects that are themselves dynamically linked. From what I understand, supporting dynamic linking requires the target to use a pic model by default. This is actually why the ci is failing with: "targets that support dynamic linking must use the `pic` relocation model" It seems we need to keep the current default. While this target is somewhat unique, I suspect this requirements here exists for good technical reasons. As an alternative solution, what if we added a note in the target documentation recommending the static relocation model for most use cases, along with an example cargo config showing how to set this up? |
I believe people can still override the default, no? |
They can ovveride the relocation model, which is what you are "supposed" to do right now. But afaik the target is what decides if dynamic relocation is supported or not. |
dc93f1d
to
6bbedd0
Compare
That makes sense. In that case, I will leave it as PIE by default. I added that to the documentation. |
LGTM |
@@ -14,6 +14,11 @@ This target is cross compiled and can be built from any host. | |||
|
|||
This target has no support for host tools, std, or alloc. | |||
|
|||
One of the primary motivations of the target is to write a dynamic linker and libc in Rust. | |||
For that, the target defaults to position-independent code and position-independent executables (PIE) by default. | |||
To be able to run a PIE, you need a dynamic linker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is quite true, since C programs compiled with -static-pie
work with no dynamic linker. What about something like this instead?
To be able to run a PIE, you need a dynamic linker. | |
PIE binaries need relocation at runtime. This is usually done by the dynamic linker or libc. |
And maybe something to the effect of "Since the point of this target is to not require those things, you may not want a PIE binary."
PR description and PR diff do not seem to be in sync any more. |
Without a standard library, we cannot unwind, so it should be panic=abort by default.
Additionally, it does not have std because while it is Linux, it cannot use libc, which std uses today for Linux.
Thirdly, relocation-model=pie is a very funny default because it forces users to write their own dynamic loader, which is fun, but probably not the most appropriate default for this target. If someone wants to write a dynamic loader, they should, but they should explicitly ask for PIE.
fixes #134763
cc @morr0ne, as you added the target, and @Noratrieb, who reviewed that PR (:D).