Skip to content

Conversation

@Vanille-N
Copy link
Contributor

@Vanille-N Vanille-N commented May 11, 2023

Although it was not in the tests, mem::transmute works for UnsafeCell -> & as well.

Draft: will also introduce more test cases for cases that fail.
Draft: depends on the new error messages from #2888

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 12, 2023
@Vanille-N
Copy link
Contributor Author

Selection of fail tests to run both TB and SB is done, I'll push when I've verified all the errors

@RalfJung
Copy link
Member

Oh, you are doing this for all fail tests now, wow. Time to rename the PR I guess? :D

@Vanille-N Vanille-N changed the title Update the TB transmute-unsafecell with the symmetrical transmute too TB: more fail tests (mostly shared with SB) May 19, 2023
@Vanille-N
Copy link
Contributor Author

Error messages updated after the merge of #2888.

Waiting for comments on

  • structure (I picked tests/fail/{tree,stacked,both}_borrows which minimizes files moved around, but we could also replicate the src structure with tests/fail/borrow_tracker/{tree_borrows,stacked_borrows,}
  • coverage (is this enough tests ? are there more things that should be tested ?)
  • other wording details

@Vanille-N Vanille-N marked this pull request as ready for review May 31, 2023 12:21
@Vanille-N
Copy link
Contributor Author

Some renames have been wrongly detected as deletion+creation. I can eventually make sure that git properly sees that it's all already existing files

@RalfJung
Copy link
Member

Some renames have been wrongly detected as deletion+creation. I can eventually make sure that git properly sees that it's all already existing files

git decides this via some heuristic, the data model doesn't even distinguish these cases so this is git diff making the decisions. I am not aware of a way to fix this. It's also not a big deal though.

@RalfJung
Copy link
Member

structure (I picked tests/fail/{tree,stacked,both}_borrows which minimizes files moved around, but we could also replicate the src structure with tests/fail/borrow_tracker/{tree_borrows,stacked_borrows,}

Hm... in pass we have {tree,stacked}-borrows so let's match that I guess? Though _ seems more consistent indeed.

both_borrows sounds a bit odd but I also can't think of anything better. 🤷

coverage (is this enough tests ? are there more things that should be tested ?)

Coverage looks pretty good to me. Which tests did you not copy over from fail/stacked_borrows?

@Vanille-N
Copy link
Contributor Author

Vanille-N commented May 31, 2023

Of all tests remaining exclusive to the fail/stacked_borrows/ folder:

Pass TB, test added

  • fnentry_invalidation
  • pass_invalid_mut
  • return_invalid_mut
  • static_memory_modification

These tests pass TB and are copied in tests/pass/tree_borrows/sb_fails.rs. A modified version that fails TB is in tests/fail/tree_borrows/.

Pass TB, no test added

  • aliasing_mut{1,2,3,4} (rely on function entry to trigger an error, which TB accepts because all mutable references are just Frozen)
  • buggy_as_mut_slice (as_mut_ptr pattern)
  • drop_in_place_{protector,retag} (zero-size retagging)
  • exposed_only_ro (wildcard pointer)
  • fnentry_invalidation2 (error message regression check)
  • illegal_read{1,2,3,4,5,6,7} (read invalidating read)
  • illegal_read_despite_exposed{1,2} (wildcard pointer)
  • illegal_write2 (write invalidating raw pointer)
  • illegal_write4 (testing write invalidation by reading)
  • illegal_write_despite_exposed1 (wildcard pointer)
  • interior_mut{1,2} (I... don't fully understand these...)
  • invalidate_against_protector1 (not invalidated by read)
  • load_invalid_mut (not invalidated for reading)
  • mut_exclusive_violation2 (no write)
  • return_invalid_mut_{option,tuple} (same as return_invalid_mut)
  • shared_rw_borrows_are_weak1 (ultra SB specific)
  • track_caller (testing error message for a passing test)
  • transmute-is-no-escape (I have no idea what this is testing)
  • unescaped_{local,static} (reference does not invalidate raw copies)
  • zst_slice (out of bounds)

Fail TB, no test added

  • box_exclusive_violation1
  • buggy_split_at_mut
  • deallocate_against_protector
  • disable_mut_does_not_merge_srw
  • illegal_dealloc1
  • illegal_read8
  • illegal_write3
  • pointer_smuggling
  • raw_tracking
  • issue-miri-1050-{1,2}
  • retag_data_race_{read,write}
  • shared_rw_borrows_are_weak2
  • shr_frozen_violation1

Most of those I skipped, I felt that either (1) they were testing some super-specific SB thing (e.g. full of comments explaining how because this SharedRO is below this other one something will happen), or (2) we already have TB tests for these (e.g. deallocation with protector is checked independently by both SB and TB).

What I could do is merge more tests so that there aren't as many things tested by both independently, select more SB fails to put in TB pass, and grab some more where both fail.

@Vanille-N
Copy link
Contributor Author

Oh no, need to rebase.
Fixes (2ebc752) is the first commit after we talked yesterday.

The only conflict seems to be the addition of #![allow(cast_ref_to_mut)] in tests/fail/shr_frozen_violation1.rs, it has been replicated in tests/fail/both_borrows/shr_frozen_violation1.rs. The newly added tests/fail/tree_borrows/write_to_shr.rs exhibits exactly the same issue and has also received an #[allow(_)].

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2023

That is everything we discussed taken care of, I assume?
All right, then please squash the history a bit (or entirely). Then you can run bors r=RalfJung.

@bors delegate

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2023

Or you could if I could type the right command...
@bors delegate+

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2023

@bors ping

@bors
Copy link
Contributor

bors commented Jun 3, 2023

😪 I'm awake I'm awake

- reorganize tests/ structure: {stacked,tree,both}_borrows
- UnsafeCell transmutation (the one that should fail, i.e. transmute &
  -> UnsafeCell then try to write)
- select TB pass tests from existing SB fail tests (and a version that
  fails TB)
- many fail tests now shared
    * extra test for TB that parent write invalidates child reads
    * buggy_* tests now shared
    * tests for deep retagging (pass_invalid_shr_*) now shared
    * extra TB test that shared references are read-only
    * aliasing_mut{1,2,3,4} adapted to fail both
    * extra TB test that write to raw parent invalidates shared children
    * mut_exclusive_violation2 now shared
    * issue-miri-1050-2 revisions fix
- deduplications
@saethlin
Copy link
Member

saethlin commented Jun 3, 2023

@bors delegate=Vanille-N

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2023

@bors delegate+

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2023

This is ready to land now anyway. 🤷 Let's see if that works...
@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2023

📌 Commit 05640c4 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 3, 2023

⌛ Testing commit 05640c4 with merge 18662da...

@bors
Copy link
Contributor

bors commented Jun 3, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 18662da to master...

@bors bors merged commit 18662da into rust-lang:master Jun 3, 2023
@Vanille-N Vanille-N deleted the tb-mut-transmute branch July 26, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: Waiting for the PR author to address review comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants