tree borrows: implicit writes#4947
Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
|
r? @RalfJung |
rustbot
added
S-waiting-on-author
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
S-waiting-on-review
@rustbot ready |
There was a problem hiding this comment.
Next round of comments. There are not that many left. :)
(But you apparently missed one comment in my previous review.)
@rustbot author
rustbot
added
S-waiting-on-author
|
Please also update the PR description; it no longer reflects the current state of this PR. |
add llvm writable attribute conditionally This PR tries to address rust-lang/unsafe-code-guidelines#584 (comment). It is part of a bachelor thesis supervised by @JoJoDeveloping and @RalfJung, for more information, see: [Project_Description.pdf](https://github.com/user-attachments/files/26537277/Project_Description.pdf). If the new `-Zllvm-writable` flag is set, the [llvm writable attribute](https://llvm.org/docs/LangRef.html#writable) is inserted for all mutable borrows. This can be conditionally turned off on a per-function basis using the `#[rustc_no_writable]` attribute. The new Undefined Behaviour introduced by this can detected by Miri, which is implemented here: rust-lang/miri#4947. Two library functions already received the `#[rustc_no_writable]` attribute, as they are known to cause problems under the Tree Borrows aliasing model with implicit writes enabled.
|
While looking over the code, an issue came forward which is mentioned in #4947 (comment). 29d5336 tries to address this issue by "constraining" the outside permission to Reserved if it is Unique. A test that currently breaks (but should not), but with the changes not anymore, has been implemented in |
|
All requested changes should be implemented now. |
rustbot
added
S-waiting-on-review
|
I will try to have another look over the entire code soon ™️ but it's a very busy week for me. |
| // `noalias` would not be sound. | ||
| Permission::new_reserved_frz() | ||
| } else { | ||
| Permission::new_reserved_im() |
There was a problem hiding this comment.
@RalfJung I don't think it makes sense to mark Boxes as ReservedIM, does it? It's what the logic was doing before and we likely didn't notice... (Note that Minirust does the same).
There was a problem hiding this comment.
Why does it not make sense? Box<T> as a function argument should be treated the same as &mut T, just without the protector.
There was a problem hiding this comment.
this is about Box in non-function-argument position, i.e. when moved inside a method.
There was a problem hiding this comment.
What is your proposal for what their initial state should be?
There was a problem hiding this comment.
just Reserved. For mutable references we have ReservedIM because of the safe code in this test, but I don't think that is possible with boxes.
There was a problem hiding this comment.
Interesting. We figured it'd be like &mut because "why not".
I'd say lets leave this unchanged for now, but please open an issue or PR (depending on what you have time for) specifically for this change, also including an example of code that you think should have UB but currently doesn't because of this.
There was a problem hiding this comment.
@RalfJung We added the tree_iwrites revision to some of the fail tests. There are many more fail tests that we could add this to. Is there a reason why not to do so?
Specifically, this should probably be on all tests in fail/both_borrows and on selected tests in fail/tree_borrows even outside the fail/tree_borrows/implicit_writes folder...
There was a problem hiding this comment.
You already added this is more than enough places I think. The pass tests are more critical since the flag makes TB less permissive.
I had another look at it and am now happy with it 🎉 (except for the things noted above) |
There was a problem hiding this comment.
Thanks! Just a few more comments for the tests, the rest looks good. :)
@rustbot author
rustbot
added
S-waiting-on-author
|
all done @rustbot ready |
rustbot
added
S-waiting-on-review
|
This looks great, thanks! Please squash the commits. You can squash manually if there are multiple independent commits you want to preserve, or use @rustbot author |
rustbot
added
S-waiting-on-author
This PR tries to address rust-lang/unsafe-code-guidelines#584 (comment). It is part of a bachelor thesis supervised by @JoJoDeveloping and @RalfJung, for more information, see: https://github.com/user-attachments/files/26537277/Project_Description.pdf. This implements the checking for implicit writes for Tree Borrows. It is disabled by default but can be enabled using the `-Zmiri-tree-borrows-implicit-writes` flag. When it is enabled, Miri inserts a write for all mutable borrows on function entry. This enables the optimization implemented here: rust-lang/rust#155207
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
4 participants
This PR tries to address rust-lang/unsafe-code-guidelines#584 (comment). It is part of a bachelor thesis supervised by @JoJoDeveloping and @RalfJung, for more information, see: Project_Description.pdf.
This implements the checking for implicit writes for Tree Borrows. It is disabled by default but can be enabled using the
-Zmiri-tree-borrows-implicit-writesflag.When it is enabled, Miri inserts a write for all mutable borrows on function entry. This enables the optimization implemented here: rust-lang/rust#155207