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

Make test suite pass under Tree Borrows #179

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

JoJoDeveloping
Copy link
Contributor

Tree Borrows is a proposed replacement for Stacked Borrows, which is more permissive in general, but introduces new UB in edge cases. This commit fixes one such edge case.

Summary of the PR

Tree Borrows is a proposed replacement for Stacked Borrows, or at least an alternative aliasing model. It tries to fix some of Stacked Borrows shortcomings, and is in general more relaxed. There are however some areas where it has more UB than Stacked Borrows, and this crate seems to exhibit one of these examples. The fix is very small, and the problem arose because the code unnecessarily created a reference where none should have been created.
I did however not read the entire code to find if this is the only place this pattern occurs in; though it is the only place within the test suite, apparently.

Checking for compatibility with Tree Borrows requires setting MIRIFLAGS="-Zmiri-tree-borrows" as an environment variable. Be aware that this is currently quite slow, we're working on making it faster.

Requirements

Most of these don't apply, but I hope the commit message/text is satisfactory.

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@NunoDasNeves
Copy link
Collaborator

@JoJoDeveloping The change seems fine, but I think the commit message could be improved.

Here's a suggestion:

Fix UB with Tree Borrows aliasing model in test suite

Tree Borrows is a proposed replacement for Stacked Borrows,
which is more permissive in general, but introduces new UB
in edge cases.

Fix one such edge case by replacing reference coercion to
a raw pointer with ptr::addr_of_mut!.

i.e. describe (briefly) what the actual fix is doing. Feel free to reword if something isn't quite right.

@liuw
Copy link
Member

liuw commented Nov 13, 2024

@JoJoDeveloping please let us know if you're going to make the requested change. We can fix it on our side, too. Your will still be the author of the said change.

Tree Borrows is a proposed replacement for Stacked Borrows,
which is more permissive in general, but introduces new UB
in edge cases.

Fix one such edge case by using ptr::addr_of_mut! to get
the address of a place without creating a reference.

Signed-off-by: Johannes Hostert <[email protected]>
@JoJoDeveloping
Copy link
Contributor Author

Here you go, I reworded the commit message. Thanks for the reminder, sorry for letting you wait.

@liuw
Copy link
Member

liuw commented Nov 14, 2024

Here you go, I reworded the commit message. Thanks for the reminder, sorry for letting you wait.

No problem at all. Thank you for your contribution!

@liuw liuw merged commit ca14646 into rust-vmm:main Nov 14, 2024
6 checks passed
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.

4 participants