CTFE eval_fn_call: use FnAbi to determine argument skipping and compatibility #91342
Conversation
|
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
We can't, because whether to pass the argument or not can vary across a |
This comment has been minimized.
This comment has been minimized.
| // For comparing the PassMode, we allow the attributes to differ | ||
| // (e.g., it is okay for NonNull to differ between caller and callee). | ||
| // FIXME: Are there attributes (`call::ArgAttributes`) that do need to be checked? | ||
| let mode_compat = || { |
There was a problem hiding this comment.
I think there should be a strict mode option that is closer to the old behavior to catch cases where someone depends on details of the rust abi.
There was a problem hiding this comment.
The new behavior is strictly more strict than the old one, so... I am confused by this comment.
There was a problem hiding this comment.
The old behavior was to say that both are incompatible when the type doesn't match and either the rust abi isn't used or the rust abi used but it isn't an argument with the direct or pair pass mode. This is much stricter than considering the argument to be compatible when the pass modes match (minus arg attributes).
There was a problem hiding this comment.
For example the old code would consider struct Foo(u8, u64, u8); and struct Bar(u64, u8, u8); to be incompatible, but the new code will consider them compatible for the rust abi as both get a cast pass mode that passes a 64bit argument (for the u64) and a 16bit argument (packing both u8 fields together).
There was a problem hiding this comment.
We still also compare the types -- I first thought comparing the modes would be enough but they do not contain remotely enough information (e.g., two Indirect arguments might have completely different types).
The idea is that the old type-based restrictions apply plus the mode has to be the same. Possibly the type-based restrictions could be relaxed (just requiring size and alignment to be the same, or even just the size) -- but so far I am not proposing to do that.
There was a problem hiding this comment.
After this PR the amount of cases is drastically increased.
For the Rust ABI, it is not. I am confused why you say that. The old conditions must be true and the pass mode must match.
Given that FnAbi does not contain the Abi, I also don't think we should make anything depend on the Abi.
There was a problem hiding this comment.
It also leads to more code being accepted for rust_abi where the indirect and cast pass modes were previously always considered incompatible.
layout_compat implements exactly the old layout checks (except it ignores the Abi), and the requirement for compatibility is layout_compat() && pad_compat() && mode_compat(). So strictly fewer things are accepted. You keep repeating that more things are accepted but that's not what the code does... or I am terribly misunderstanding you.
So, no, what you say is just not true I think. Previously we did not even look at modes so indirect and cast could easily have been considered compatible!
There was a problem hiding this comment.
In the rust abi it is unstable if an argument will be passed as indirect or cast. I can't think of any valid reasons to allow mismatched types for them even for libstd internal usage.
We accept transmutes between repr(Rust) types whose fields happen to be laid out in the right way. I think we likewise should accept function calls whose FnAbi happens to match. I think anything else would be (a) inconsistent, and (b) not even well-defined since we don't have a clear spec for what is guaranteed to hold true about the Rust ABI into the future.
There was a problem hiding this comment.
I wish I could replace layout_compat by something more principled that reflects whether under current rustc, the data would actually be passed properly, but I don't know nearly enough about all this FnAbi madness to implement such a check. Unfortunately, ignoring the layout entirely is wrong (PassMode does not capture enough information), and comparing it for full equality is also wrong. It doesn't look like we actually have an abstraction that properly represents if arguments are compatible. Based on what @eddyb had said in the past I thought FnAbi provides that but now I don't think this is the case.
There was a problem hiding this comment.
Specifically, I am very confused about two points:
- Your summary even says that after this PR, for the Rust ABI, we accept strictly less than before. Before it was 'types equal or compatible scalar or scalar pair', now it is 'layout, padding, and pass mode compatible'. Here, 'layout compatible' is defined as 'types equal or compatible scalar or scalar pair', i.e., exactly the old definition. Do we agree so far? (I am assuming that equal types implies equal pass mode and padding; if that is not the case your summary is incorrect.)
So I don't see why it would make any sense to introduce an even stricter mode with this PR. (I also don't think we should even have that mode, or at least I don't want to be the one implementing it -- but even leaving that aside, it is orthogonal to this PR.) - It is true that this PR accepts some calls that were previously rejected, namely calls for non-Rust ABIs where the types are different but the layout is compatible and the padding and pass mode match. This might be worth debating (it is the only change here that I see as potentially contentious), but I don't see a good reason to single out the Rust ABI now that we have pass mode information available. From what I understand, however, your 'strict mode' is not about this case.
apiraino
added
the
T-compiler
| _ret, | ||
| _unwind, | ||
| )? | ||
| .map(|(body, _instance)| (body, instance))); |
There was a problem hiding this comment.
I'm not happy about this hack but it is needed to keep the const-time backtraces the way they were...
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
fn-abi
branch
2 times, most recently
from
094f199
to
70066b2
Compare
|
Okay I think this should be mostly set now -- except for @bjorn3's concern about a 'strict mode' which I have not understood yet. |
|
Did a review pass, too. Considering that and the comments above: @bors r=eddyb,oli-obk |
|
📌 Commit 56b7d5f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 56b7d5f with merge 17d141cdaad042d4541355ae007ce2b2279a0a6e... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
There's no way this can affect @bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@59337cd. Direct link to PR: <rust-lang/rust#91342> 💔 miri on windows: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk). 💔 miri on linux: test-pass → build-fail (cc @RalfJung @eddyb @oli-obk).
|
Finished benchmarking commit (59337cd): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
adjust for FnAbi changes This is the Miri side of rust-lang/rust#91342.
This makes use of the
FnAbitype in CTFE/Miri, which @eddyb has been saying for years is what we should do.^^FnAbiis used toI was hoping it would also simplify the code, but that is not the case -- the previous type compatibility checks are still required (AFAIK), only the ZST skipping is gone and that took barely any code. We also need some hacks because
FnAbiassumes a certain way of implementingcaller_location(by passing extra arguments), but Miri can just read the caller location from the call stack so it doesn't need those arguments. (The fact that every backend has to separately implement support for these arguments seems suboptimal -- looks like this might have been better implemented on the MIR level.) To avoid having to implement those unnecessary arguments in Miri, we just compute whether the argument is present on the caller/callee side, but don't actually pass that argument around.I have no idea if this looks the way @eddyb thinks it should look... but it makes Miri's test suite pass. ;)
One of rustc's tests fails unfortunately (
ui/const-generics/issues/issue-67739.rs), some const generic code that is evaluated too early -- I think that should raiseTooGenericbut instead it ICEs. My assumption is this is some FnAbi code that has not been properly tested on polymorphic code, but it might also be me calling that FnAbi code the wrong way.r? @oli-obk @eddyb
Fixes #56166
Miri PR at rust-lang/miri#1928