-
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
repr(int) fieldless enums are ABI-compatible with int #128600
Conversation
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
@maurer do you have any concerns with this guarantee from a CFI perspective? Does CFI accept or reject calls where caller and callee disagree on whether to use a |
Your change as it stands won't break anything, but it also won't make CFI across ABI-compatible enums succeed, so it would have the downside that there would be more behavior that is "legal Rust but not CFI legal", which we generally want to minimize as much as possible. This does go against how CFI works in C/C++ - there, it's not legal to perform the function pointer casts you're legalizing here, so it would be different. It also does lessen the protections somewhat, as distinct enums with the same The tag for If we want this ABI compatibility to extend to CFI, we should add a case to the transform which changes |
Thanks! Given that I've not touched the CFI code before, I'd prefer if you could take care of that -- either as a parallel PR (I could then cherry-pick your patches) or as a follow-up. |
OK. This does technically weaken CFI, as it means that in this case:
If an attacker got control over the I'm a firm believer that CFI should only enforce the language rules, so if we're going to flatten enums like this for purposes of function pointer casts, we'll need to weaken CFI like this, but I do want to point out that it will weaken CFI in Rust. It might weaken it enough that it might make sense to have this behavior flagged. |
cc @rcvalle to avoid any surprise conflicts when it comes to implementation. The tl;dr is that Ralf is legalizing this cast:
and we'd have to weaken (possibly behind a flag) the Rust CFI support so that it doesn't trigger on it. |
Without my CFI hat on, what is your plan for the semantics of such casts in the event that they receive an out-of-range value for the enum? |
Right, that's why this is flagged for t-lang discussion and it is why I asked for your input for that discussion. :) We could also declare this to be "erroneous behavior", where we say that if this happens it is considered a bug, but detecting the bug is not required by implementations. If the bug is not detected, the call proceeds as-if there was a transmute. @chorman0773 suggested this should be documented as guaranteed; not sure if they have a concrete usecase in mind or whether it just seems "obvious". Note that we already declare
That's already documented as part of the general ABI compatibility rules: it acts like a transmute, so for out-of-range values the call is still UB. |
As with |
Shouldn't x-lang be a non-issue, as those all need to be |
The ABI compatibility rules apply to all ABIs, including |
AFAIK we guarantee that |
It seems these reprs aren't mutually exclusive (see https://doc.rust-lang.org/reference/type-layout.html#combining-primitive-representations-of-enums-with-fields-and-reprc); otherwise yes, we could add it to the integer normalization option for CFI for non |
@RalfJung would, for example, #[repr(C, u8)] and #[repr(u8)] be ABI compatible? |
That is an odd combination. The docs say
but for |
If |
I don't see how the C/C++ pointer aliasing rules should be relevant here. We aren't even talking about pointer types here, and furthermore Rust deliberately does not want C/C++-style "strict aliasing" rules, so Rust-to-Rust calls should not be subject to such rules even when they go via This PR is entirely about by-values passing of enums, let's stick to that in the discussion here. |
If that is the case, I think we could use @maurer's suggestion and add it to the integer normalization option for CFI for non |
I said the |
Aliasing is relevant here because CFI is essentially about labeling function pointers based on aliasing rules. If I make a function pointer out of a Rust I'm not suggesting we add strict-aliasing semantics to Rust, but make it possible for Rust to communicate with a language which assumes it. |
Could we filter out by |
@RalfJung I was mostly asking a general question as I was formalizing the abi compat rules - whether or not |
The implementation looks fine, though I'm also curious on the answer to
On the compiler side, we seem to separate the caller ty and the callee ty, so it would be possible to make this one-direction only. (i.e. legalize only |
I have already answered this above and it is answered in the stable docs:
|
I don't understand what you mean by "filter out". However, when I try to use this
So I think we do not have to worry about |
f5b183f
to
6e42535
Compare
That seems like something to be added to some sort of FFI documentation explaining our cross-language ABI compatibility rules. |
☔ The latest upstream changes (presumably #128761) made this pull request unmergeable. Please resolve the merge conflicts. |
6e42535
to
82ee3cd
Compare
Isn't this a problem with all kinds of "normalization", e.g. |
Marking as waiting on team. |
I'm in favor of C says that enumerated types are compatible with their underlying type. That would suggest that we should make I'm also not sure exactly how compatibility is determined between C and Rust. Do we ignore the tag equivalence requirement so that At an absolute minimum, C Footnotes
|
I'm going to close this PR... it probably doesn't make a lot of sense to discuss this in isolation; instead, there is a wider discussion to be had about CFI vs ABI compatibility. Also see:
This probably warrants a t-lang design meeting, but I don't currently have the capacity to drive something like that. |
Given that we already allow mixing
char
andu32
, as well asusize
/isize
and the corresponding fixed-size type, it seems odd to not also do this for enums. However, if we declare this legal, then Rust code may start relying on it, so arguably CFI should also allow this, which could miss some real bugs where the mismatch is accidental.So we have to decide -- do we say this is ABI-compatible, or do we declare this to be "erroneous behavior": similar to overflow in (non-wrapping) arithmetic, such behavior is always a bug, but implementations are not required to detect the bug (with a panic or abort), they can also continue execution in some well-defined way. (For the ABI question, that well-defined way is to transmute the argument from the caller type to the callee type. This is well-defined in the sense that we say exactly what happens, but it may be UB if the caller uses e.g.
u32
and the callee expects arepr(u32)
enum for which the chosen argument value is not valid.)I am not sure if we are already guaranteeing anything like this somewhere. (The nomicon talks about these
repr
flags affecting the ABI, but the nomicon is not normative, and also it's not entirely clear whether ABI here also includes function call concerns.)Cc @rust-lang/lang @rust-lang/opsem
This is part of a wider set of discussions around CFI vs ABI compatibility: