-
Notifications
You must be signed in to change notification settings - Fork 352
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
intptrcast: remove information about dead allocations #3122
Conversation
47febce
to
fde8582
Compare
Uh, what... I was sure I ran the tests locally... |
fde8582
to
cd16e5e
Compare
Okay, |
cf83d11
to
75b92dc
Compare
That's because we need the GC to be able to delete entries from |
Yes. |
src/intptrcast.rs
Outdated
// We can *not* remove this from `base_addr`, since `addr_from_alloc_id` is called on each | ||
// attempt at a memory access to determine the allocation ID and offset -- and there can | ||
// still be pointers with `dead_id` that one can attempt to use for a memory access. |
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.
I don't understand why this is problematic for accesses. Even if we remove an allocation which still has pointers to it, the pointer is still dangling right? I expected the problem here to be converting pointers to freed allocations to integers.
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.
When such a dangling pointer gets used, the first thing that happens is that we call ptr_get_alloc
, which is expected to return the AllocId
and the offset inside the allocation. Once we remove an allocation from base_addr
we can no longer determine the offset, so we have a problem.
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.
In what case would we need to know the offset inside the allocation and also be okay with using a pointer to a deallocated allocation?
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.
You are implicitly suggesting a rework of the Machine
trait here. And it's not super trivial since in error messages we want to distinguish "ptr with no provenance" from "ptr with provenance of a dangling allocation", so just returning None
when asked for "what alloc_id + offset is this provenance" would not work.
So this can probably be done but it's a bigger change than what this PR does. So far the API is designed around the idea that "get alloc_id + offset" and "determine whether alloc is live and offset inbounds" are two separate independent steps. That's a nice and simple interface; we'd have to make it more complicated if we want to be able to remove pointers from base_addr
on deallocation.
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.
You are implicitly suggesting a rework of the Machine trait here.
I didn't mean to suggest that. I think this whole situation you're describing is part of why I was so confused, I don't know the interpreter internals well enough to reason from them as opposed to from the language. And to this specific question of mine above, I was half expecting you to produce some obscure Rust code in answer.
Can you adjust this comment a bit to indicate that the design of the interpreter requires us to be able to do "get alloc_id + offset" before we do UB detection? Perhaps something like
// We can *not* remove this from `base_addr`, since the interpreter design requires that we be able to retrieve
// an AllocId + offset for any memory access *before* we check if the access is valid.
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 with any such tweak
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.
Ah okay. :) I incorporated that into the comment.
ca60134
to
f22fede
Compare
@saethlin any objections to landing this, any remaining ideas for how to improve this? I think making |
…ting the base address
☔ The latest upstream changes (presumably #3130) made this pull request unmergeable. Please resolve the merge conflicts. |
f22fede
to
1a91227
Compare
1a91227
to
ab68ed7
Compare
@bors r=saethlin |
☀️ Test successful - checks-actions |
The discussion in #3103 convinced me we don't have to keep
int_to_ptr_map
around for dead allocations. But we should not make that depend on the GC, we can just tie it to when the allocation gets freed. That means everything still behaves deterministically, if anything weird happens (but it shouldn't).r? @saethlin
Only the first and last commit contain logic changes, the 2nd commit just moves code around a bit.