-
Notifications
You must be signed in to change notification settings - Fork 13k
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
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) |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if let Some(pointee) = layout.pointee_info_at(cx, offset) { |
Everything else is mostly relying on rustc_target
doing the heavy lifting, already.
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.
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.
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.
r=me modulo what @eddyb says about the rust_abi
check
if !rust_abi { | ||
// Don't risk anything | ||
return false; | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
…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
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)