Remove ability to disable some target features #116584
Conversation
Change the features enabled by various target specifications from defaults to mandatory features. This will be used to prevent -C target-feature=-x87 and -C target-feature=-sse from changing the ABI on x86.
These features enable registers that are or will be used in the ABI. Instead of only being implied by the target cpu, they are now explicitly enabled so that they cannot be disabled by -C target-feature.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
These commits modify compiler targets. |
|
? @rustbot label: +A-abi +O-x86 +T-opsem |
rustbot
added
A-ABI
|
I'm not familiar enough with this code to review. |
This alone is not a reason to make the change, that comment is from 2014 and it's not a public documentation, exactly. Why does it make sense to have non-removable features for targets? If yes, then are all features currently specified in target specs supposed to be non-removable? |
rustbot
added
S-waiting-on-author
Because some can change the call ABI, and this makes changing the features incompatible with the prescribed call ABI, especially for
Only ones that change ABI must be considered immutable. Specifically, we need to know what the "baseline" ABI is supposed to be for the target and for that knowledge to not disappear. If LLVM supported different ways of handling this problem, we could adopt them, but afaik it does not. |
This seems like it would imply that |
|
It doesn't, because I was talking about the features of the target CPU, not the features of the target ISA. |
|
With this PR, what happens when someone does
This is one half of resolving #116344, so that issue should hopefully provide the necessary context. But yeah I agree this needs some form of process, t-compiler MCP at least. |
|
Why is the feature stability mechanism insufficient for this? If you don't want to make it possible to disable them (at which point they are ABI-critical and it doesn’t make sense to make it possible to enable a disabled feature either, since it would also change the ABI) just don’t make the feature stably controllable via
I have trouble understanding what this sentence means. Neither LLVM nor Rust has a concept of target ISA features, as far as I know? One way or another everything comes down to a single set of flags, whether they are configured implicitly (e.g. through |
| // the host target are overridden by `-Ctarget-cpu=*`. On the other hand, what about when both | ||
| // `--target` and `-Ctarget-cpu=*` are specified? Both then imply some target features and both | ||
| // flags are specified by the user on the CLI. It isn't as clear-cut which order of precedence | ||
| // should be taken in cases like these. |
There was a problem hiding this comment.
I don’t believe this change addresses this FIXME at all. It still isn’t at all clear to me, even if we agreed that we don’t want to allow some features to be configurable outside of --target specifications, that this specific way to build up the feature set is the absolutely the correct one.
For example:
rustc -Ctarget-cpu=native --target=x86_64-unknown-linux-gnu
pretty clearly shows that the intent is for the -Ctarget-cpu=native to take precedence over whatever x86_64-unknown-linux-gnu means (including it not having any of the fancy AVX extensions.)
On the other hand
rustc -Ctarget-cpu=pentium4 --target=x86_64v4-unknown-linux-gnu
is quite a bit more ambiguous: does the user want something that would run on a pentium4? Or something that would run on an x86_64v4 class hardware, which pentium4 is not? Should the ordering of flags be taken into the account (unusual for rustc, but standard behaviour in gcc/clang style CLIs.) Perhaps it would be better to just error outright?
|
I'll try to play a devil's advocate here.
ABI differences turn into unsafety only if functions with one set of features are called as functions with another incompatible set of features. Maybe if we are trying to improve safety, it should be done in a more fine-grained way too. |
I wasn't aware of that, but
|
The problem is that, as Ralf highlighted, very few people actually understand the safety requirements. The majority of users only know that they can "solve a problem" by enabling/disabling features, they don't necessarily understand the larger ramifications of that. Users can and do set incompatible sets of features that fail to even pass LLVM's isel. I would even argue that very few compiler engineers understand the safety requirements, given the number of ABI bugs we've had along these lines.
That could work for
In the model I was implicitly referencing, a given CPU implements a specific instance of an ISA, with certain ISA extensions, and |
Yes, that should either be a separate target or such calls should be disallowed. (Not just unsafe, they are always UB so allowing them makes little sense even in unsafe code.) Usually, "softfloat" is a separate target, so that would be a good pattern for x86 to follow as well. |
|
That's already what Please refer to the target spec: rust/compiler/rustc_target/src/spec/x86_64_unknown_none.rs Lines 21 to 23 in 4b85902 |
|
If we actually want to support this configuration being set for any given And if it is unsupported, and we're okay with it being UB to enable, then why are we allowing people to set an option that would be effectively guaranteed to make all linkage to the Rust core library virtually instantly undefined behavior, no ifs, ands, or buts? |
|
Setting Or, more likely, they recompile libcore for the kernel anyways, thus reinventing |
|
Sounds like it's likely some people have lurking bugs because of this. Is there any way to prevent linking against code compiled with a different ABI? Is any metadata about the target embedded in the object files? |
|
And... people keep rediscovering this property, again and again, after thrashing with the compiler and their setups for HOURS. The fact that these combinations of flags are de facto unviable has bitten learned academics who specialize in verification of instruction sets: https://alastairreid.github.io/verifying-vectorized-code2/ Apologies if anyone thinks I should have stated this earlier! I refrained, because I could probably go on like this for what might well be the entire rest of the actual year, until 2024 January 1. Thus, simply stating "no one actually understands this" seemed enough. |
The answer to that is a familiar, joyous refrain: |
Yes. Make sure that all code built for the same target is ABI-compatible, no matter the target features. Reject any code or target features where we can't make sure this is the case. Linking files with different targets is already "obviously" wrong. (I don't know if we actually prevent it on a technical level though.) That's exactly what this PR does. So, looks like we should get this to be decided by some team. But is it t-compiler or t-lang? Or both? |
|
Given that stabilizing target features seems to be a t-lang thing, I am going to assume the same applies here. @rust-lang/lang, we'd like to remove the ability to disable certain target features. The broad goal we are going for here is that code built for the same target is ABI-compatible no matter the target features. This is an easy rule to remember and a principle that it seems reasonable to enforce. This principle is currently violated in several ways (for x86, that's tracked in #116344 and #116558). This PR is one part of achieving that, this pre-RFC is another part, and then one final piece is needed to reject This particular PR fixes #116344 for our tier 1 targets. The issue is that disabling the "x87" feature on an x86 (32bit or 64bit) target changes the ABI, so linking two object files that were compiled with different target features can lead to UB in entirely safe code. In particular, calling any Disabling these target features also does not guarantee code that does not use SSE/x87 registers, since the standard library was built with those registers available. SSE registers are used even in code that doesn't involve any floats. So if the goal is to e.g. not save SSE registers in a context switch, then disabling the SSE target feature does not suffice, you also need to rebuild the standard library. In other words, EDIT: I have created a design meeting proposal for the wider problem space here, in case you think this needs more fundamental discussion. |
RalfJung
added
T-lang
|
Implementation-wise, will this PR also reject enabling |
I think we'll need a list of forbidden features for each target (complementing the list of required features), then we can use that to reject |
|
Hm, it still seems fairly fragile that each x86 target has to list those required/forbidden features though. Maybe we should use a different approach where features in the features list are marked based on whether they are "float-ABI-affecting" and then we reject changing those features? (We also have SIMD-ABI-affecting features but there's a different proposal for how to handle them so that we can still support toggling those features.) The tricky part with this approach is figuring out whether |
| .split(',') | ||
| .filter(|v| !v.is_empty() && backend_feature_name(v).is_some() && v.starts_with("+")) | ||
| .map(String::from), | ||
| ); |
There was a problem hiding this comment.
So this will silently ignore -Ctarget-features=-x87? That doesn't seem great, we shouldn't just ignore that. We should hard error or at least warn about ignoring this flag.
I think we'll want to compare that in the final feature set, certain features (for x86: x87, soft-float; for other targets we need to still determine this list) have the same state as without applying the user config. This needs to take into account -Ctarget-cpu and features implying other features.
|
Comments in rustc led me to believe that this minimal change could unblock #115919, but the code implementing the problematic design is almost entirely in LLVM. Target, target-cpu and target-feature together specify the CPU features the generated code will use, and the ABI follows from that. rustc could work around that to try to make the ABI follow only from target alone, but that requires at minimum a complete list of all ABI-affecting features in all targets, and maybe more. A clean solution would have the complete logic in rustc instead. That's a bunch of code for a small reduction in unsoundness. To trigger the problem, you have to link an object compiled with different target-feature-flags into the program. I think "link an object" is the important part, not "different target-feature-flags". The object could also have been compiled with a different "--target" flag, or a different rustc version, or a pascal compiler, or it could have been downloaded from the internet. Making linking objects into the program safer would be nice in general, but there are far too many ways in which problematic objects can be produced. That said, giving an error message for some known-problematic arguments to |
|
@GuentherVIII I want to
And for what we have decided on so far, we can pretty much decide if things are being used correctly in a fairly likely way for most cases and emit errors in the known-problem cases, yes. It's true that it may be a "best effort" in reality, but that's to be expected, and it describes the "99%" case, even amongst the set of "advanced" users. It's not necessarily about sound or unsound, it's about user experience, so that we aren't sitting smugly on an answer we already have and letting people waste hundreds of hours on trying to resolve cryptic issues. It also is about the questions already raised re: actually being able to use optimizations in the compiler without exploding things in the face of users. We reserve the right to change the Rust ABI between versions of the compiler without notice, but that means that if setting |
That's describing a lot of rustc. ;)
As Jubilee said: this happens every time someone uses
Correct. But that doesn't seem so bad? Such features are fairly rare, in particular if we assume we can exclude We can start with the tier 1 targets, and then slowly work our way through the rest. |
|
☔ The latest upstream changes (presumably #117716) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This question is scheduled for a T-lang design meeting on 2023-12-20: |
|
@rustbot labels -I-lang-nominated We discussed this question in the T-lang design meeting on 2023-12-20. The consensus was generally in favor of fixing this, and we were interested in seeing more work in this direction. While we considered various ways that this could be fixed, we were supportive of finding the most minimal, simplest way to fix this as the first step, assuming that such an approach proves to be feasible. We'll need to see at least the results of a crater run and further analysis to confirm that feasibility. We appreciate @RalfJung and all the others who have been investing time to push this forward. |
rustbot
removed
the
I-lang-nominated
|
@GuentherVIII any updates on this? thanks |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
The documentation of TargetOptions.features claims that these features will always be passed to LLVM, but the logic implemented in #83084 allows them to be disabled with -C target-feature. Change the implementation to match the documentation.
Disabling x87 on x86 changes the way functions return floats. The second commit makes x87 mandatory on the existing i686 targets. Disabling floating point usage now requires a new target. #116344 does not fully get fixed by this (softfloat targets still have a problem with
+x87), but it helps a lot.In preparation for #115919 sse and sse2 are also made mandatory.
See https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Float.20ABI.20hell/near/394473707 and #116344.
Lang team: see nomination comment below.
@rustbot label: +A-abi +O-x86 +T-opsem +T-compiler