Skip to content

miri/const eval: support MaybeDangling#150446

Open
WaffleLapkin wants to merge 1 commit intorust-lang:mainfrom
WaffleLapkin:miri-maybe-dangling
Open

miri/const eval: support MaybeDangling#150446
WaffleLapkin wants to merge 1 commit intorust-lang:mainfrom
WaffleLapkin:miri-maybe-dangling

Conversation

@WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Dec 27, 2025

View all comments

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.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@theemathas
Copy link
Contributor

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

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
@WaffleLapkin WaffleLapkin force-pushed the miri-maybe-dangling branch 2 times, most recently from e5b7d00 to 959b31b Compare February 26, 2026 14:28
@WaffleLapkin WaffleLapkin marked this pull request as ready for review February 26, 2026 16:28
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2026
@WaffleLapkin
Copy link
Member Author

@RalfJung I think I've addressed the review comments and this is ready for review too :)

(still blocked on the compiler change though)

@RalfJung
Copy link
Member

(still blocked on the compiler change though)

By "the compiler change" you mean #150447 ?

@WaffleLapkin
Copy link
Member Author

yes

@rust-bors

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2026

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in tests/codegen-llvm/sanitizer

cc @rcvalle

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. PG-exploit-mitigations Project group: Exploit mitigations labels Mar 5, 2026
@rustbot

This comment has been minimized.

@WaffleLapkin WaffleLapkin force-pushed the miri-maybe-dangling branch from ac559e4 to d14294e Compare March 5, 2026 11:20
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Mar 5, 2026

Please rebase so it's easier to see the diff. :) Also, CI doesn't seem entirely happy yet.
@rustbot author

@rustbot rustbot removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 5, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2026

Error: shortcut handler unexpectedly failed in this comment: failed to add labels

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

@RalfJung RalfJung added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 5, 2026
@apiraino
Copy link
Contributor

apiraino commented Mar 5, 2026

(retry for debugging)

@rustbot author

@Kobzol
Copy link
Member

Kobzol commented Mar 5, 2026

(It was a 500 GitHub error)

@WaffleLapkin WaffleLapkin force-pushed the miri-maybe-dangling branch from d14294e to 955b127 Compare March 6, 2026 11:12
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2026

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.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks! All nits are minor.

View changes since this review

/// If this is `Some`, then `reset_provenance_and_padding` must be true (but not vice versa:
/// we might not track data vs padding bytes if the operand isn't stored in memory anyway).
data_bytes: Option<RangeSet>,
may_dangle: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc comment.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't even a borrow checking thing. Please move the test to tests/fail/validity and have it disable Stacked Borrows to ensure we don't rely on the aliasing model for catching this.

}
}
ty::Adt(adt, _) if adt.is_maybe_dangling() => {
let could_dangle = mem::replace(&mut self.may_dangle, true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let could_dangle = mem::replace(&mut self.may_dangle, true);
let old_may_dangle = mem::replace(&mut self.may_dangle, true);

Ub(DanglingIntPointer { addr: 0, .. }) => NullPtr { ptr_kind, maybe: false },
Ub(DanglingIntPointer { addr: i, .. }) => DanglingPtrNoProvenance {
ptr_kind,
// FIXME this says "null pointer" when null but we need translate
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME this says "null pointer" when null but we need translate

While we're at it

Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make sense any more since we're skipping that part when may_dangle is true.

Comment on lines 544 to 545
// Make sure this is non-null. We checked dereferenceability above, but if `size` is zero
// that does not imply non-null.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Make sure this is non-null. We checked dereferenceability above, but if `size` is zero
// that does not imply non-null.
// Make sure this is non-null. This is obviously needed when `may_dangle` is set,
// but even if we did check dereferenceability above that would still allow null
// pointers if `size` is zero.

Copy link
Member

@RalfJung RalfJung Mar 6, 2026

Choose a reason for hiding this comment

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

Given that this case was contentious, please also add a test like

// Under the current models, we do not forbid writing through
// `MaybeDangling<&i32>`. That's not yet finally decided, but meanwhile
// ensure we document this and notice when it changes.
fn write_through_shr(x: MaybeDangling<&i32>) {
  let y: *mut i32 = transmute(x);
  y.write(1);
}

let mutref = &mut 0i32;
write_through_shr(transmute(mutref));

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin WaffleLapkin force-pushed the miri-maybe-dangling branch 2 times, most recently from c052e2f to 7b01b7c Compare March 6, 2026 15:33
@WaffleLapkin WaffleLapkin force-pushed the miri-maybe-dangling branch from 7b01b7c to 4a26802 Compare March 6, 2026 16:38
@WaffleLapkin
Copy link
Member Author

@RalfJung I think I addressed your nits, unless I missed something ^^'

Copy link
Member

Choose a reason for hiding this comment

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

I meant for this to just be a new test in the other file -- no reason to split it up.

self.check_wide_ptr_meta(place.meta(), place.layout)?;
}
// Make sure this is dereferenceable and all.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Determine size and alignment of pointee.

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

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. PG-exploit-mitigations Project group: Exploit mitigations 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

Development

Successfully merging this pull request may close these issues.

7 participants