tree borrows: implicit writes by quiode · Pull Request #4947 · rust-lang/miri

quiode · 2026-04-08T14:08:23Z

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-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

rustbot · 2026-04-08T14:08:27Z

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

quiode · 2026-04-08T14:08:38Z

r? @RalfJung

…attribute locally

RalfJung

I didn't look at the tests yet, and have to dig more into the logic, but here are some first comments.

View changes since this review

RalfJung

And here's the second round, now also with comments for the logic and the tests. :) Please ask either in this PR, or Johannes on Zulip, if you have questions.

View changes since this review

RalfJung · 2026-04-10T13:04:23Z

@rustbot author

rustbot · 2026-04-10T13:04:26Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

… from permission

quiode · 2026-04-17T12:08:15Z

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 array-pointer-access.rs. Also added additional assertions when using is_initial() as this now allows Unqiue which some functions do not expect.

RalfJung

Thanks for the revision! However I am confused by some of the changes.

View changes since this review

RalfJung · 2026-04-17T14:48:47Z

I have pushed a refactor of the permission logic that is a bit longer than what you had but IMO also easier to follow. Please let me know what you think.

@JoJoDeveloping

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.

@JoJoDeveloping

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.

…ussed

quiode · 2026-04-21T15:33:50Z

All requested changes should be implemented now.
@rustbot ready

JoJoDeveloping

the test is good, but I have some comments mostly affecting the things beside the code that is run as part of this test. (I.e. it's mostly bikeshedding, sorry.)

View changes since this review

JoJoDeveloping · 2026-04-21T19:30:14Z

I will try to have another look over the entire code soon ™️ but it's a very busy week for me.

JoJoDeveloping · 2026-04-24T09:07:45Z

+                        // `noalias` would not be sound.
+                        Permission::new_reserved_frz()
+                    } else {
+                        Permission::new_reserved_im()


@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).

View changes since the review

Why does it not make sense? Box<T> as a function argument should be treated the same as &mut T, just without the protector.

this is about Box in non-function-argument position, i.e. when moved inside a method.

What is your proposal for what their initial state should be?

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.

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.

JoJoDeveloping · 2026-04-24T09:13:33Z

@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...

View changes since the review

You already added this is more than enough places I think. The pass tests are more critical since the flag makes TB less permissive.

JoJoDeveloping · 2026-04-24T09:27:19Z

I will try to have another look over the entire code soon ™️ but it's a very busy week for me.

I had another look at it and am now happy with it 🎉 (except for the things noted above)

RalfJung

Thanks! Just a few more comments for the tests, the rest looks good. :)

@rustbot author

View changes since this review

RalfJung · 2026-04-27T12:36:07Z

@@ -0,0 +1,30 @@
+// Tests that accessing the same array using disjoint pointers does not cause UB under Tree Borrows.
+// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.


Suggested change

// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.

// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.

// In particular, in implicit_writes mode we used to set the "outside" permissions to

// `Unique`, which caused an ICE.

// Put differently, this checks the absence of subobject provenance for arrays.

RalfJung · 2026-04-27T12:39:21Z

+
+fn main() {
+    let mut x: Box<u8> = Box::new(0u8);
+    let ptr = &mut *x as *mut u8;


There's still a mutable reference here. If that isn't relevant, maybe use &raw mut *x instead?

RalfJung · 2026-04-27T12:41:41Z

@@ -1,4 +1,6 @@
-//@compile-flags: -Zmiri-tree-borrows


You can keep this line so that all revisions use TB.

RalfJung · 2026-04-27T12:44:26Z

This is quite confusing to be in this folder as this test is about not having implicit writes.

Maybe call it tests/pass/tree_borrows/no_implicit_writes.rs?

RalfJung · 2026-04-27T12:45:00Z

This can be merged with tests/pass/tree_borrows/implicit_writes/ptr_write.rs.

RalfJung · 2026-04-27T12:48:37Z

Maybe make this just a function in tests/pass/tree_borrows/tree-borrows.rs? That's where we have most of these kinds of tests.

JoJoDeveloping

some other cosmetic things in tests I noticed while reading over this with Ralf.

View changes since this review

JoJoDeveloping · 2026-04-27T12:39:23Z

can you rename this test into retag_is_data_race.rs or something like that? The current name is in praticular wrong as it is not a "read" retag.

JoJoDeveloping · 2026-04-27T12:40:51Z

+    thread::yield_now(); // force other thread to read first
+
+    fn write(y: &mut u8, v: u8, barrier: &Arc<Barrier>) {
+        //~^ ERROR: Data race detected


can you make the error message more specific, to match on the retag being called a write?

tree borrows: strong mode

0c4cd6e

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Apr 8, 2026

rustbot assigned RalfJung Apr 8, 2026

fix errors

9f91666

quiode marked this pull request as draft April 8, 2026 17:11

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 8, 2026

quiode added 2 commits April 8, 2026 20:27

add global ignore flag to more tests as as_mut_ptr no longer has the …

7ef4918

…attribute locally

fix failing tests

7f8b705

RalfJung reviewed Apr 9, 2026

View reviewed changes

quiode added 2 commits April 10, 2026 13:20

implicit writes off by default

229edaf

use byte slice instead of str for better compatibility across targets

982b2e1

RalfJung reviewed Apr 10, 2026

View reviewed changes

Comment thread tests/fail/tree_borrows/implicit_writes/ptr_write.rs

resolve multiple comments on pr

7826cfa

quiode marked this pull request as ready for review April 10, 2026 13:15

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Apr 10, 2026

RalfJung reviewed Apr 10, 2026

View reviewed changes

Comment thread src/borrow_tracker/mod.rs

quiode added 7 commits April 10, 2026 16:44

remove access information from new permission and calculate it purely…

39aeb2b

… from permission

move tree_borrows variable out of the loop

fcf5844

iwrites -> tree_iwrites

a62a19c

remove flag.rs

8589573

add clarification in test doc

a9c4860

remove mut_option test

0826d53

remove ptr_write2 test

abda111

RalfJung reviewed Apr 17, 2026

View reviewed changes

Comment thread src/borrow_tracker/tree_borrows/tree.rs Outdated

Comment thread tests/fail/tree_borrows/implicit_writes/ptr_write.rs Outdated

Comment thread tests/fail/tree_borrows/implicit_writes/ptr_write_unsafe_cell.rs Outdated

Comment thread tests/pass/tree_borrows/array-pointer-access.rs

refactor new permission logic

d44b84c

RalfJung force-pushed the tb-strong-mode branch from de4a6ef to d44b84c Compare April 17, 2026 14:48

remove unnecessary debug assert

8a28ee9

quiode added 5 commits April 20, 2026 14:42

improve comments

715d7a1

add flag to test, improve wording in comment

ae94051

update passing tests for TB to also test for implicit writes, as disc…

8e72a1c

…ussed

exclude failing targets from read_retag_no_race

211bc15

remove implicit writes revision from retag no race test

005c0d5

JoJoDeveloping reviewed Apr 21, 2026

View reviewed changes

Comment thread tests/pass/tree_borrows/read_retag_no_race.rs Outdated

quiode added 2 commits April 21, 2026 16:04

check boxes for implicit writes the same as mutable references

b00b3c6

read_retag test for implicit writes

2a947c6

quiode requested a review from JoJoDeveloping April 21, 2026 15:06

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Apr 21, 2026

JoJoDeveloping suggested changes Apr 21, 2026

View reviewed changes

trim read_retag test to the essentials

cce5ab7

JoJoDeveloping reviewed Apr 24, 2026

View reviewed changes

RalfJung reviewed Apr 27, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 27, 2026

JoJoDeveloping approved these changes Apr 27, 2026

View reviewed changes

		@@ -0,0 +1,30 @@
		// Tests that accessing the same array using disjoint pointers does not cause UB under Tree Borrows.
		// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.

-// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.
+// The pointer will access the memory range using the outside permission, thus testing if it has been set correctly.
+// In particular, in implicit_writes mode we used to set the "outside" permissions to
+// `Unique`, which caused an ICE.
+// Put differently, this checks the absence of subobject provenance for arrays.

Conversation

quiode commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 8, 2026

Uh oh!

quiode commented Apr 8, 2026

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Apr 10, 2026

Uh oh!

rustbot commented Apr 10, 2026

Uh oh!

Uh oh!

Uh oh!

quiode commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Apr 17, 2026

Uh oh!

Uh oh!

quiode commented Apr 21, 2026

Uh oh!

JoJoDeveloping left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoJoDeveloping commented Apr 21, 2026

Uh oh!

JoJoDeveloping Apr 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

quiode commented Apr 8, 2026 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading

RalfJung left a comment •

edited by rustbot

Loading

quiode commented Apr 17, 2026 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading

JoJoDeveloping left a comment •

edited by rustbot

Loading

JoJoDeveloping Apr 24, 2026 •

edited by rustbot

Loading

JoJoDeveloping Apr 24, 2026 •

edited by rustbot

Loading

JoJoDeveloping commented Apr 24, 2026 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading

JoJoDeveloping left a comment •

edited by rustbot

Loading