slightly improve protector-related error messages

[RalfJung]

I find the current retag messages confusing, since they sound like the item was protected, when it still actively is protected (and that is, in fact, the issue).

Example error message:

error: Undefined Behavior: not granting access to tag <3095> because incompatible item [Unique for <3099>] is protected by call 943
  --> tests/fail/stacked_borrows/invalidate_against_barrier1.rs:5:25
   |
5  |     let _val = unsafe { *x }; //~ ERROR: protect
   |                         ^^ not granting access to tag <3095> because incompatible item [Unique for <3099>] is protected by call 943
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3095> was created by a SharedReadWrite retag at offsets [0x0..0x4]
  --> tests/fail/stacked_borrows/invalidate_against_barrier1.rs:10:16
   |
10 |     let xraw = &mut x as *mut _;
   |                ^^^^^^
help: <3095> cannot be used for memory access because that would remove protected tag <3099>, protected by this function call
  --> tests/fail/stacked_borrows/invalidate_against_barrier1.rs:1:1
   |
1  | / fn inner(x: *mut i32, _y: &mut i32) {
2  | |     // If `x` and `y` alias, retagging is fine with this... but we really
3  | |     // shouldn't be allowed to use `x` at all because `y` was assumed to be
4  | |     // unique for the duration of this call.
5  | |     let _val = unsafe { *x }; //~ ERROR: protect
6  | | }
   | |_^
help: <3099> was derived from <3098>, which in turn was created here
  --> tests/fail/stacked_borrows/invalidate_against_barrier1.rs:12:17
   |
12 |     inner(xraw, xref);
   |                 ^^^^
   = note: backtrace:
   = note: inside `inner` at tests/fail/stacked_borrows/invalidate_against_barrier1.rs:5:25
note: inside `main` at tests/fail/stacked_borrows/invalidate_against_barrier1.rs:12:5
  --> tests/fail/stacked_borrows/invalidate_against_barrier1.rs:12:5
   |
12 |     inner(xraw, xref);
   |     ^^^^^^^^^^^^^^^^^

r? @saethlin

[saethlin]

I'm imagining someone updating their toolchain, looking at this new error and thinking "What does it mean to activate a tag? Is that a new thing in Stacked Borrows?" and I'm coming up empty-handed for where they could look to figure out what "activate" means. I can't find it in the implementation, or in the markdown file that we link in error messages.

So I'd prefer that our documentation somewhere be spruced up to at least mention the word in the way that you are using it, or that we word things a bit differently to avoid this sentence construction. Instead of " cannot be activated" perhaps "access through cannot be permitted"?

[RalfJung]

Yeah, that's fair, we never explain what 'activate' means... I have changed it to "cannot be used for memory access".

Is "derived" an okay term to use?

[saethlin]

I think that "derived" is okay. It's at least mentioned in the markdown documentation that we link to in a sentence that helps explain what it means, and LLVM uses it occasionally.

But

because of protected derived tag

I'm not sure what the "derived' is adding here? It seems to beg the question, "derived from what?"

[RalfJung]

Derived from the tag that we cannot use for the memory access. I updated the text and the example message in the OP.

[saethlin]

tests/fail/stacked_borrows/aliasing_mut1.rs prints this:

help: <3094> cannot be used for memory access because of protected tag <3093> (transitively derived from <3094>), created here
  --> tests/fail/stacked_borrows/aliasing_mut1.rs:7:50
   |
7  |     let xraw: *mut i32 = unsafe { mem::transmute(&mut x) };
   |                                                  ^^^^^^

and now I'm even more confused by the "transitively derived from" statement. How can tag 3093 be derived from 3094, if it was created first?

[RalfJung]

Ah, I screwed up the direction...

[RalfJung]

Ah no wait. The direction is right in the message in the OP. Hm...

[RalfJung]

Hm yeah I don't think there is in general a "derived from" relationship between these tags... good catch!

[RalfJung]

There is still something odd about the error though, and it comes from somewhere in the diagnostic logic you implemented -- the error says incompatible item [Unique for <3099>] is protected but later talks about that would remove protected tag <3097>. Hu?

[RalfJung]

I don't understand the Protection type. What are tag vs orig_tag?

[saethlin]

Protectors get introduced upon function entry, which is a point at which we retag, so there are two important tags in play. The tag for the argument itself inside the function is the tag that we aren't allowed to remove. In some sense it is just how we know there is a problem. For diagnostics, I want to tell the user about which pointer they passed to the function which is being protected, and that's why we store an orig_tag in a Protection event, so that we can point to its creation event and indicate to the user that "it's this pointer" that we're protecting. Even though of course we're protecting the pointer inside the function, I don't think we have a good way of pointing to that argument in particular. That would be ideal.

[RalfJung]

Oh, so there are three tags you want to show here -- tag used for access, protected tag, parent of protected tag. Honestly that seems a bit too confusing for me? Certainly omitting the actually protected tag, as the messages do now, is very confusing.

The actually protected tag should usually be derived from the accessing tag, that's why it is further up the stack, but it would also be derived from a sibling of the accessing tag. The parent of the protected tag can have basically arbitrary relationship to the accessing tag.

I don't think we have a good way of pointing to that argument in particular. That would be ideal.

Yeah, the span for these fn_entry retags should be made to point at the argument they are retagging from.

[RalfJung]

Here is how aliasing_mut looks like now:

error: Undefined Behavior: not granting access to tag <3094> because incompatible item [Unique for <3100>] is protected by call 943
  --> tests/fail/stacked_borrows/aliasing_mut1.rs:3:1
   |
3  | pub fn safe(_x: &mut i32, _y: &mut i32) {} //~ ERROR: protect
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <3094> because incompatible item [Unique for <3100>] is protected by call 943
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3094> was created by a Unique retag at offsets [0x0..0x4]
  --> tests/fail/stacked_borrows/aliasing_mut1.rs:7:50
   |
7  |     let xraw: *mut i32 = unsafe { mem::transmute(&mut x) };
   |                                                  ^^^^^^
help: <3094> cannot be used for memory access because that would remove protected tag <3100>, protected by this function call
  --> tests/fail/stacked_borrows/aliasing_mut1.rs:3:1
   |
3  | pub fn safe(_x: &mut i32, _y: &mut i32) {} //~ ERROR: protect
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: <3100> was derived from <3094>, the tag used for this memory access
   = note: backtrace:
   = note: inside `safe` at tests/fail/stacked_borrows/aliasing_mut1.rs:3:1
note: inside `main` at tests/fail/stacked_borrows/aliasing_mut1.rs:13:5
  --> tests/fail/stacked_borrows/aliasing_mut1.rs:13:5
   |
13 |     safe_raw(xraw, xraw);
   |     ^^^^^^^^^^^^^^^^^^^^

[RalfJung]

Oh wait you actually went one further in the chain... why that? You printed event.retag.orig_tag, which is the parent of the parent of the protected tag.

[saethlin]

There's even a // FIXME in the code that sets the source_info for FnEntry retags 😂 I'm going to start looking into improving that, but I'll admit I have little experience with MIR transforms.

[RalfJung]

Yeah I wrote that about 4 years ago.^^

But meanwhile, what about this PR? I think it is an improvement, but I don't understand why you were displaying the grandparent of the protected tag.

[saethlin]

Frankly at this stage I don't either (it's possible this was just a mistake or me not understanding protectors), and the adjustment you made to that diagnostic looks like a big improvement to me.

[saethlin]

Also you renamed some but not all of the test which mention barrier. Is that intentional?

[RalfJung]

Also you renamed some but not all of the test which mention barrier. Is that intentional?

Good catch, fixed.

[saethlin]

Looking over this again, I think this change is good to go. I think that when rust-lang/rust#101111 becomes available we will be able to replace at least the last span, because the better FnEntry retag spans are a much better way to tell the user which argument was protected.

@bors r+

[bors]

📌 Commit abe890d has been approved by saethlin

It is now in the queue for this repository.

[bors]

⌛ Testing commit abe890d with merge fec1c7a...

[bors]

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing fec1c7a to master...

[RalfJung]

I think that when rust-lang/rust#101111 becomes available we will be able to replace at least the last span, because the better FnEntry retag spans are a much better way to tell the user which argument was protected.

👍 the tests will also need re-blessing then.