Skip to content

miri/const eval: support MaybeDangling #150446

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.

Sign up for GitHub

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

Jump to bottom
Draft
WaffleLapkin wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

miri/const eval: support MaybeDangling #150446

WaffleLapkin wants to merge 2 commits into from
+73 −29

Conversation

@WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Dec 27, 2025

r? RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

The Miri subtree was changed

cc @rust-lang/miri

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 27, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2025

This PR was rebased onto a different main 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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer
Copy link
Collaborator

rust-log-analyzer commented Dec 27, 2025

The job aarch64-gnu-llvm-20-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
[RUSTC-TIMING] jiff test:false 9.641
error[E0599]: no variant or associated item named `MaybeDangling` found for enum `rustc_hir::LangItem` in the current scope
   --> compiler/rustc_middle/src/ty/adt.rs:320:44
    |
320 |         if tcx.is_lang_item(did, LangItem::MaybeDangling) {
    |                                            ^^^^^^^^^^^^^ variant or associated item not found in `rustc_hir::LangItem`

For more information about this error, try `rustc --explain E0599`.
[RUSTC-TIMING] rustc_middle test:false 10.673
error: could not compile `rustc_middle` (lib) due to 1 previous error
Bootstrap failed while executing `--stage 2 test --skip compiler --skip src`

theemathas
theemathas reviewed Dec 28, 2025
View reviewed changes
compiler/rustc_const_eval/src/interpret/validity.rs
let inner = self.ecx.project_field(val, FieldIdx::ZERO)?;
self.visit_value(&inner)?;

self.may_dangle = could_dangle;
Copy link
Contributor

@theemathas theemathas Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that this line could be skipped if the previous lines produce Err?

Copy link
Member Author

@WaffleLapkin WaffleLapkin Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine. If this returns Err UB has been detected, so there is no way to continue execution anyway. See also similar pattern here, in existing code: https://github.com/WaffleLapkin/rust/blob/ae692187fd93e2aa68ed7cc6ba07cdeaee69304e/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs#L977-L979

theemathas
theemathas reviewed Dec 28, 2025
View reviewed changes
src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
let in_field = mem::replace(&mut self.in_field, true); // remember and restore old value
let may_dangle = mem::replace(&mut self.may_dangle, true); // remember and restore old value
self.walk_value(place)?;
self.may_dangle = may_dangle;
Copy link
Contributor

@theemathas theemathas Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here

@theemathas
Copy link
Contributor

theemathas commented Dec 28, 2025

Does this have insta-stable behavior change for ManuallyDrop in consteval?

@WaffleLapkin WaffleLapkin marked this pull request as draft December 28, 2025 13:42
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2025
@RalfJung
Copy link
Member

RalfJung commented Dec 28, 2025

It should only affect the behavior of code that still has UB until MaybeDangling is stabilized.

@RalfJung
Copy link
Member

RalfJung commented Jan 4, 2026

It is very import that we land #150447 before landing this, to avoid a situation where we generate LLVM IR with UB but Miri reports no UB.

@RalfJung RalfJung added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 4, 2026
RalfJung
RalfJung reviewed Jan 4, 2026
View reviewed changes
src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
// even if field retagging is not enabled. *shrug*)
self.walk_value(place)?;
}
ty::Adt(adt, _) if adt.is_maybe_dangling() => {
Copy link
Member

@RalfJung RalfJung Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just entirely skip traversing the references inside MaybeDangling? For the aliasing model that should just be a complete NOP.

RalfJung
RalfJung reviewed Jan 4, 2026
View reviewed changes
src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs
Copy link
Member

@RalfJung RalfJung Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tree Borrows will need the same patch.

RalfJung
RalfJung reviewed Jan 4, 2026
View reviewed changes
src/tools/miri/tests/pass/move_manually_drop.rs
Copy link
Member

@RalfJung RalfJung Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this test in https://github.com/rust-lang/miri/tree/master/tests/pass/both_borrows and call it maybe_dangling.rs. Also make sure it runs under both SB and TB, like the other tests in that folder.

Copy link
Member

@RalfJung RalfJung Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, probably better to directly test MaybeDangling instead of just indirectly via ManuallyDrop?

And finally, please add "fail" tests to ensure we still catch null and unaligned references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

@theemathas theemathas theemathas left review comments

Assignees

@RalfJung RalfJung

Labels

S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@WaffleLapkin @rustbot @rust-log-analyzer @theemathas @RalfJung