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

better diagnostics for Tree Borrows + int2ptr casts #3766

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

RalfJung
Copy link
Member

  • Entirely reject -Zmiri-permissive-provenance -Zmiri-tree-borrows since that combination just doesn't work
  • In the int2ptr cast warning, when Tree Borrows is enabled, do not recommend -Zmiri-permissive-provenance, instead note that Tree Borrows does not support int2ptr casts

Fixes #3764

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Jul 26, 2024

📌 Commit 4cee511 has been approved by RalfJung

It is now in the queue for this repository.

bors added a commit that referenced this pull request Jul 26, 2024
better diagnostics for Tree Borrows + int2ptr casts

- Entirely reject `-Zmiri-permissive-provenance -Zmiri-tree-borrows` since that combination just doesn't work
- In the int2ptr cast warning, when Tree Borrows is enabled, do not recommend `-Zmiri-permissive-provenance`, instead note that Tree Borrows does not support int2ptr casts

Fixes #3764
@bors
Copy link
Contributor

bors commented Jul 26, 2024

⌛ Testing commit 4cee511 with merge ed1fb37...

@bors
Copy link
Contributor

bors commented Jul 26, 2024

💔 Test failed - checks-actions

@RalfJung RalfJung force-pushed the tree-borrows-int2ptr branch from 4cee511 to 8bbd757 Compare July 26, 2024 12:44
@RalfJung RalfJung force-pushed the tree-borrows-int2ptr branch from 8bbd757 to 6f8dc27 Compare July 26, 2024 12:51
@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Jul 26, 2024

📌 Commit 6f8dc27 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 26, 2024

⌛ Testing commit 6f8dc27 with merge c84b86d...

@bors
Copy link
Contributor

bors commented Jul 26, 2024

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

@bors bors merged commit c84b86d into rust-lang:master Jul 26, 2024
8 checks passed
@RalfJung RalfJung deleted the tree-borrows-int2ptr branch July 26, 2024 13:20
This was referenced Jul 26, 2024
@ohsayan
Copy link

ohsayan commented Jul 30, 2024

Also, is there a way to suppress the int2ptr casts warning when using tree borrows?

@RalfJung
Copy link
Member Author

Not really -- since Tree Borrows doesn't support int2ptr casts, not showing the warning seems hazardous. Now that I understand that your original issue was unrelated, we could possibly partially revert this PR -- but it is still the case that Tree Borrows + int2ptr will miss a lot of UB (even more so than usual for int2ptr), and I am not sure how to best communicate that to users.

The warning should be shown only once. Is it so bad that it needs to be hidden?

@ohsayan
Copy link

ohsayan commented Jul 30, 2024

@RalfJung I see, and it makes complete sense. Actually, I've been hunting a bunch of bugs down and the int-to-ptr cast warnings filled the logs making it hard to find the actual error (which in one case was an unaligned read)...because we use them excessively for storing extra pointer metadata using alignment hacks; that's why I was wondering if there was a way around it.

Also, since we use crossbeam, as it happens, tests fail when using stacked borrows but pass using tree borrows (and it's a known issue; see crossbeam-rs/crossbeam#545). So yeah, in a bit of a situation. But I can totally see why suppressing int2ptr warnings could be dangerous.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 30, 2024

storing extra pointer metadata using alignment hacks

FWIW this can be done without int2ptr casts using strict provenance APIs, but that currently requires either nightly or using the sptr crate.

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.

Tree-borrows false positive with iter::Extend?
3 participants