drop glue takes in mutable references, it should reflect that in its type #56165
Conversation
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -196,6 +196,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> | |||
| // that will take care to make it UB to leave the range, just | |||
| // like for transmute). | |||
| caller.value == callee.value, | |||
| (layout::Abi::ScalarPair(ref caller1, ref caller2), | |||
| layout::Abi::ScalarPair(ref callee1, ref callee2)) => | |||
| caller1.value == callee1.value && caller2.value == callee2.value, | |||
There was a problem hiding this comment.
Are you not using FnType yet :P? Anyway this seems... fine.
But only for the Rust ABI so you might want to add that as a condition, if it's not already.
(ScalarPair is considered an aggregate and follows more complex rules, in non-Rust ABIs)
There was a problem hiding this comment.
Which rules for aggregates can there be when the ABIs are the same except for the ranges...? I thought these ABIs encode everything.^^
Are you not using FnType yet :P?
Nope, you didn't yet convince me that it would make anything simpler.^^
There was a problem hiding this comment.
Abi does not encode everything, no. C ABIs can depend on details of the C type definition, which is annoying but FnType contains all of that information.
Nope, you didn't yet convince me that it would make anything simpler.^^
It would make things correct, which seems more important than simplicity. It's not like that hard to use, just have to move some code from rustc_codegen_ssa and then compare ArgType pairwise.
(assuming you don't want to emulate the runtime behavior of mismatched ABIs :P)
There was a problem hiding this comment.
C ABIs can depend on details of the C type definition, which is annoying but FnType contains all of that information.
Ouch. Okay, I will restrict type mismatches to Rust Abi functions.
assuming you don't want to emulate the runtime behavior of mismatched ABIs :P
You assume correctly. :P
It's not like that hard to use, just have to move some code from rustc_codegen_ssa and then compare ArgType pairwise.
Would you mentor someone? I could open an E-mentor issue.
There was a problem hiding this comment.
Would you mentor someone? I could open an E-mentor issue.
Sure, always happy to mentor compiler cleanups!
The main issue to moving that stuff around is pointee_info_at (it would have to be moved to rustc::ty::layout, as an additional method in TyLayoutMethods):
rust/src/librustc_codegen_llvm/abi.rs
Line 487 in dae6c93
Everything else is mostly relying on rustc_target doing the heavy lifting, already.
There was a problem hiding this comment.
But only for the Rust ABI so you might want to add that as a condition
Done.
This comment has been minimized.
This comment has been minimized.
Centril
added
T-lang
| if !rust_abi { | ||
| // Don't risk anything | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This changes the behavior in the case of (Scalar, Scalar) pairs, right? (They didn't use to be dependent on the rust_abi) -- @eddyb is that what you intended?
There was a problem hiding this comment.
Yes, scalar pairs are only used in the Rust ABI, two types with the same scalar pair optimization can have incompatible by-value ABI with extern "C".
There was a problem hiding this comment.
Yes. It also changes behavior on non-Rust ABIs for Scalar, where we no longer allow any kind of mismatch -- mostly for consistency.
|
@bors r=eddyb,nikomatsakis |
|
📌 Commit d9de72f has been approved by |
bors
added
S-waiting-on-bors
…komatsakis drop glue takes in mutable references, it should reflect that in its type When drop glue begins, it should retag, like all functions taking references do. But to do that, it needs to take the reference at a proper type: `&mut T`, not `*mut T`. Failing to retag can mean that the memory the reference points to remains frozen, and `EscapeToRaw` on a frozen location is a NOP, meaning later mutations cause a Stacked Borrows violation. Cc @nikomatsakis @gankro because Stacked Borrows Cc @eddyb for the changes to miri argument passing (the intention is to allow passing `*mut [u8]` when `&mut [u8]` is expected and vice versa)
…komatsakis drop glue takes in mutable references, it should reflect that in its type When drop glue begins, it should retag, like all functions taking references do. But to do that, it needs to take the reference at a proper type: `&mut T`, not `*mut T`. Failing to retag can mean that the memory the reference points to remains frozen, and `EscapeToRaw` on a frozen location is a NOP, meaning later mutations cause a Stacked Borrows violation. Cc @nikomatsakis @gankro because Stacked Borrows Cc @eddyb for the changes to miri argument passing (the intention is to allow passing `*mut [u8]` when `&mut [u8]` is expected and vice versa)
drop glue takes in mutable references, it should reflect that in its type When drop glue begins, it should retag, like all functions taking references do. But to do that, it needs to take the reference at a proper type: `&mut T`, not `*mut T`. Failing to retag can mean that the memory the reference points to remains frozen, and `EscapeToRaw` on a frozen location is a NOP, meaning later mutations cause a Stacked Borrows violation. Cc @nikomatsakis @gankro because Stacked Borrows Cc @eddyb for the changes to miri argument passing (the intention is to allow passing `*mut [u8]` when `&mut [u8]` is expected and vice versa)
|
☀️ Test successful - status-appveyor, status-travis |
When drop glue begins, it should retag, like all functions taking references do. But to do that, it needs to take the reference at a proper type:
&mut T, not*mut T.Failing to retag can mean that the memory the reference points to remains frozen, and
EscapeToRawon a frozen location is a NOP, meaning later mutations cause a Stacked Borrows violation.Cc @nikomatsakis @gankro because Stacked Borrows
Cc @eddyb for the changes to miri argument passing (the intention is to allow passing
*mut [u8]when&mut [u8]is expected and vice versa)