Emit retags in codegen to support BorrowSanitizer (part 2)#156210
Emit retags in codegen to support BorrowSanitizer (part 2)#156210icmccorm wants to merge 3 commits into
Conversation
rustbot
added
S-waiting-on-review
|
|
|
r? saethlin |
This comment has been minimized.
This comment has been minimized.
icmccorm
force-pushed
the
codegen-emit-retag-2
branch
from
951a9ed to
4e3df71
Compare
This comment has been minimized.
This comment has been minimized.
|
Now that #154327 has been merged, we no longer need special handling for drop glue. |
This comment has been minimized.
This comment has been minimized.
icmccorm
force-pushed
the
codegen-emit-retag-2
branch
from
4e3df71 to
5135340
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| LocalRef::PendingOperand => LocalRef::PendingOperand, | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| // If we branched during retagging, then we need to update the |
There was a problem hiding this comment.
How would retagging introduce a branch?
There was a problem hiding this comment.
We selectively retag variants of enums that contain references or Box. For example,
fn foo(opt: Option<Box<i32>>) { .. }This will branch on the discriminant of opt to retag the Box, if it's there.
| } | ||
| fx.store_return(bx, ret_dest, &fn_abi.ret, invokeret); | ||
|
|
||
| // If the return value has variants that needed to be retagged, |
There was a problem hiding this comment.
Why are you mentioning variants here? Why isn't this just "if the return value needs to be retagged"?
There was a problem hiding this comment.
Yeah, that should just say "if the return value needs to be retagged".
| use crate::traits::BuilderMethods; | ||
|
|
||
| pub(crate) fn place_needs_retag(place: &Place<'_>) -> bool { | ||
| // We're not interested in tracking stores to "outside" locations |
There was a problem hiding this comment.
What does "outside" mean?
There was a problem hiding this comment.
Whups - we shouldn't need to need to skip these retags any more, now that #154341 has been merged. This was an artifact of the old AddRetag implementation. Here's the relevant part of @RalfJung's explanation for why these were skipped in that PR:
For assignments of the form
*ptr = expr, if the assignment involves copying a reference, we probably want to do a retag -- but if we do a Retag(*ptr) as the next instruction, it can be non-trivial to argue that this even retags the right value, so we refrained from doing retags in that case.
I'll remove place_needs_retag.
5 participants
Tracking issue: #154760
Zulip Thread
This is one of several PRs that will add experimental support for emitting retags as function calls in codegen. Each PR will be a minimal, improved slice of the changes in #155965.
This PR adds a new unstable flag
-Zcodegen-emit-retag, which will enable experimental retag calls in generated code. This flag is a nop for now, but the relevant methods have been added to codegen_ssa, and they are called wherever retags are necessary. Subsequent PRs will complete this implementation.This does not depend on #156208.
r? @RalfJung