Skip to content
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

Improper but "silently succeeding" use of post write barrier #1129

Closed
wks opened this issue Apr 28, 2024 · 1 comment · Fixed by #1130
Closed

Improper but "silently succeeding" use of post write barrier #1129

wks opened this issue Apr 28, 2024 · 1 comment · Fixed by #1130

Comments

@wks
Copy link
Collaborator

wks commented Apr 28, 2024

This is a sub-problem of #1038 which we decided to postpone after the MEP for removing ObjectReference::NULL (#1043).

MMTk's write barrier functions take target: ObjectReference as parameter. For example,

pub fn object_reference_write_pre<VM: VMBinding>(
    mutator: &mut Mutator<VM>,
    src: ObjectReference,
    slot: VM::VMEdge,
    target: ObjectReference,
) {
    mutator
        .barrier()
        .object_reference_write_pre(src, slot, target);
}

With #1043 merged, this form no longer works if target is NULL or any other non-reference values. But real-world programs may overwrite a field previously holding an object reference with NULL or a non-reference value.

However, all bindings are still working. That's because they transmute 0 to ObjectReference in C/C++ and pass them to Rust. Currently, the object-remembering write barrier simply ignores the slot and the target fields, so it is unaware that the ObjectReference variable is holding an illegal value. But we have to acknowledge that it is a bug and should be fixed.

We can simply change target to Option<ObjectReference>, with None representing a deleted edge.

However, object_reference_write will no longer work if target is None because MMTk core cannot write NULL or non-reference values to the slot.

pub fn object_reference_write<VM: VMBinding>(
    mutator: &mut Mutator<VM>,
    src: ObjectReference,
    slot: VM::VMEdge, // Edge::store(&self, target: ObjectReference) does not accept null target, which is intentional.
    target: ObjectReference,
);
@wks
Copy link
Collaborator Author

wks commented Apr 28, 2024

An alternative solution is that object_reference_write_{pre,post} still have target: ObjectReference parameter, but the VM binding shall not call object_reference_write_pre if the old value in the slot is not an object reference, and shall not call object_reference_write_post if the new value written to the slot is not an object reference.

This scheme should work for the generational barrier because the generational barrier is an insertion barrier. It only cares about newly added edges from old to young objects.

It should work for the snapshot-at-the-beginning (SATB) barrier, too. It only tries to remember the old target if it exists.

It should work for naive RC, too. If there is no old target, there is no object to decrement its RC; if there is no new target, there is no object to increment its RC, either.

But if we want to implement deferred RC, we need an object-remembering barrier. It needs to keep a snapshot of the object and put the object reference in the modbuf in the pre-barrier even if we are updating a field from NULL to a target, so that we can apply the right number of inc and dec operations at the next GC.

wks added a commit to wks/mmtk-core that referenced this issue Apr 28, 2024
Change the target type of write barrier functions to
`Option<ObjectReference>`.  This allows VM bindings to use the write
barrier API when storing NULL references or non-references values to
slots.

Fixes: mmtk#1129
github-merge-queue bot pushed a commit that referenced this issue Apr 30, 2024
Change the target type of write barrier functions to
`Option<ObjectReference>`. This allows VM bindings to use the write
barrier API when storing NULL references or non-references values to
slots.

Fixes: #1129
@wks wks closed this as completed in #1130 Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant