Skip to content

Show the offset, length and memory of uninit read errors #142673

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 18, 2025

r? @RalfJung

I want to improve memory dumps in general. Not sure yet how to do so best within rust diagnostics, but in a perfect world I could generate a dummy in-memory file (that contains the rendered memory dump) that we then can then provide regular rustc Spans to. So we'd basically report normal diagnostics for them with squiggly lines and everything.

@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 Jun 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Comment on lines -666 to +674
throw_ub!(InvalidUninitBytes(None));
throw_ub!(InvalidUninitBytes(match op.to_op(self)?.as_mplace_or_imm() {
Left(mplace) => mplace.ptr().provenance.and_then(|prov| {
let start = mplace.ptr().into_parts().1;
let size = op.layout().size;
let range = alloc_range(start, size);
Some((prov.get_alloc_id()?, BadBytesAccess { access: range, bad: range }))
}),
Right(_) => None,
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one doesn't show up in diagnostics, but it seemed good to change it, too

Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't it show up in diagnostics? Aren't all the miri output diffs caused by exactly this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, that diff is probably caused by the read_scalar change.

If this logic here can't be tested, I'd rather remove it, given that it is currently wrong due to how it uses into_parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#142839 gets rid of 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 is read_immediate, #142839 does nothing there...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, I mixed up things.

hmm. yea I think I just tried this one because I thought it should be hit somewhere. So I'll turn it into a span_bug

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 18, 2025

grml. I haven't been able to run miri's dep tests in-tree in forever. Always sth about libc not found. Will try to bless the others and hope none of these need blessing

@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Comment on lines -666 to +674
throw_ub!(InvalidUninitBytes(None));
throw_ub!(InvalidUninitBytes(match op.to_op(self)?.as_mplace_or_imm() {
Left(mplace) => mplace.ptr().provenance.and_then(|prov| {
let start = mplace.ptr().into_parts().1;
let size = op.layout().size;
let range = alloc_range(start, size);
Some((prov.get_alloc_id()?, BadBytesAccess { access: range, bad: range }))
}),
Right(_) => None,
}))
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't it show up in diagnostics? Aren't all the miri output diffs caused by exactly this?

throw_ub!(InvalidUninitBytes(None));
throw_ub!(InvalidUninitBytes(match op.to_op(self)?.as_mplace_or_imm() {
Left(mplace) => mplace.ptr().provenance.and_then(|prov| {
let start = mplace.ptr().into_parts().1;
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct -- depending on which type of provenance this is, start will be either relative to the allocation, or absolute. into_parts has a doc comment warning about this. :) Maybe we should rename it into_raw_parts or so to make it more clear that this API is somewhat dicey.

@bors
Copy link
Collaborator

bors commented Jun 27, 2025

☔ The latest upstream changes (presumably #143091) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants