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

TrustCell fixes #223

Merged
merged 4 commits into from
Feb 6, 2023
Merged

TrustCell fixes #223

merged 4 commits into from
Feb 6, 2023

Conversation

Imberflur
Copy link
Contributor

@Imberflur Imberflur commented Feb 5, 2023

Fixing some unsoundness, see commit messages.

I'm hoping to eventually replace TrustCell with atomic_refcell::AtomicRefCell, however that should probably wait on bholley/atomic_refcell#18

Copy link
Contributor Author

@Imberflur Imberflur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some notes to help reviewers

Comment on lines +62 to +65
miri:
name: "Miri"
runs-on: ubuntu-latest
continue-on-error: true # Needed until all Miri errors are fixed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Miri to the CI, however it still runs into some errors in the MetaTable code that is handling vtable pointers. So I set continue-on-error: true for now (hopefully this does what I think it does...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it seems like there is no way to not show the red X in the PR list: https://github.com/orgs/community/discussions/15452

Comment on lines +235 to +237
black_box(world.fetch::<DeltaTime>());
black_box(world.fetch::<VecStorage<Pos>>());
black_box(world.fetch::<VecStorage<Spring>>());
Copy link
Contributor Author

@Imberflur Imberflur Feb 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added black_box just to be more sure that the compiler doesn't optimize these out (didn't really see much difference in the benchmark though)

Comment on lines +641 to +652
// https://github.com/rust-lang/rust/issues/63787
#[test]
fn drop_and_borrow_in_fn_call() {
fn drop_and_borrow(cell: &TrustCell<u8>, borrow: Ref<'_, u8>) {
drop(borrow);
*cell.borrow_mut() = 7u8;
}

let a = TrustCell::new(4u8);
let borrow = a.borrow();
drop_and_borrow(&a, borrow);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to reproduce the scenario in the issue linked in the comment, however Miri was already reporting UB due to retagging of the reference by the mem::forget call in RefMut::map.

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 this pull request may close these issues.

2 participants