GC: factor out visiting all machine values #2559
Conversation
RalfJung
force-pushed
the
gc-refactor
branch
from
0321a55
to
cf59ffe
Compare
|
More bits of machine state that are not visited:
|
|
We will probably want a trait VisitMachineValues {
fn visit_machine_values(&self, impl FnMut(&Operand<Provenance>));
} |
|
I can read what you changed, but I don't understand how this helps with missing pieces of state in the interpreter? |
RalfJung
force-pushed
the
gc-refactor
branch
from
cf59ffe
to
c9a425d
Compare
|
I think separating the visiting from the remaining GC logic makes it easier to check that nothing was forgotten. Also using the visitor pattern with exhaustive field lists in the |
RalfJung
force-pushed
the
gc-refactor
branch
from
c9a425d
to
cd13ad3
Compare
|
I am happy with the structure of this PR now. @saethlin do you want to take over and complete the PR by implementing all the missing things in the visitor? Or should we land this and you hopefully can do that in a future PR? |
RalfJung
force-pushed
the
gc-refactor
branch
from
cd13ad3
to
7054800
Compare
| @@ -628,6 +624,34 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| impl VisitMachineValues for ThreadManager<'_, '_> { | |||
| fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>)) { | |||
| // FIXME some other fields also contain machine values | |||
There was a problem hiding this comment.
I see there are other fields with machine values, but it's not clear to me that for example thread_local_alloc_ids is keeping pointers live. Are there other fields?
There was a problem hiding this comment.
Isn't that the only way you will reach the current value of thread-local variables? And a thread-local variable can store a pointer to somewhere else.
There was a problem hiding this comment.
It's similar, I think, to the global base tag map in Stacked Borrows -- I don't know how your GC is not failing left and right due to removing the base tags.
There was a problem hiding this comment.
Stack::retain never removes the bottom-most tag. (I forgot about this until just now)
There was a problem hiding this comment.
Ah. Yeah that would help but needs to be carefully documented.
|
I'd prefer to do a second PR. r=me with or without addressing the formatting nit (I think we have other formatting jank which will snap into place when the style team gets off the ground). |
RalfJung
force-pushed
the
gc-refactor
branch
from
7054800
to
8dc4893
Compare
|
@bors r=saethlin |
|
@bors ping |
|
😪 I'm awake I'm awake |
|
@bors retry |
|
@bors r=saethlin |
|
💡 This pull request was already approved, no need to approve it again. |
|
@bors r- |
|
@bors r=saethlin |
|
☀️ Test successful - checks-actions |
Expand VisitMachineValues to cover more pointers in the interpreter Follow-on to #2559 This is making me want to write a proc macro 🤔 r? `@RalfJung`
Expand VisitMachineValues to cover more pointers in the interpreter Follow-on to #2559 This is making me want to write a proc macro 🤔 r? `@RalfJung`
Expand VisitMachineValues to cover more pointers in the interpreter Follow-on to rust-lang/miri#2559 This is making me want to write a proc macro 🤔 r? `@RalfJung`
@saethlin that is roughly what I had in mind.
I think some parts of the state are skipped by the visitor. I listed the ones that I found in FIXMEs but I am not sure if that list is complete.