Skip to content

fix handling of base address for TypeId allocations #144187

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix handling of base address for TypeId allocations #144187

wants to merge 1 commit into from
+55 −43

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 19, 2025 •
edited
Loading

This fixes the problems discovered by @theemathas in #142789:

  • const-eval would sometimes consider TypeId pointers to be null
  • the type ID is different in Miri than in regular executions

Both boil down to the same issue: the TypeId "allocation" has a guaranteed 0 base address, but const-eval assumes it was non-zero (like normal allocations) and Miri randomized it (like normal allocations).

r? @oli-obk

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 2025

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@RalfJung RalfJung mentioned this pull request Jul 19, 2025
Open
RalfJung
RalfJung commented Jul 19, 2025
View reviewed changes
compiler/rustc_const_eval/src/const_eval/machine.rs
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing really changes here, but I looked at the code to ensure it is correct for TypeId and realized some ways in which it, and the comments, could be improved.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the type-id-base-addr branch from 31b35e0 to 2ad5678 Compare July 19, 2025 20:17
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025
@theemathas theemathas mentioned this pull request Jul 20, 2025
Open
@theemathas
Copy link
Contributor

Is it intentional that this PR does not fix the offset_from behavior, which can assert whether two TypeIds are the same?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2025

Is it intentional that this PR does not fix the offset_from behavior, which can assert whether two TypeIds are the same?

Yes, catching UB is not a requirement for CTFE as it has significant impact on compile times. A program that does UB if two type ids do not match may suddenly stop erroring with the next compiler release, or error even if the type ids match.

@RalfJung
Copy link
Member Author

Also, you can't use it to get a bool in the code that tells you the answer, you can only use it to abort compilation, so it's less of a hazard IMO.

@RalfJung RalfJung force-pushed the type-id-base-addr branch from 2ad5678 to 3f9be40 Compare July 20, 2025 20:14
@RalfJung
Copy link
Member Author

#144169 landed so this PR is no longer blocked.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 21, 2025

📌 Commit 3f9be40 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jul 21, 2025
@jieyouxu
Rollup merge of rust-lang#144187 - RalfJung:type-id-base-addr, r=oli-obk
a5d520e 
fix handling of base address for TypeId allocations

This fixes the problems discovered by `@theemathas` in rust-lang#142789:
- const-eval would sometimes consider TypeId pointers to be null
- the type ID is different in Miri than in regular executions

Both boil down to the same issue: the TypeId "allocation" has a guaranteed 0 base address, but const-eval assumes it was non-zero (like normal allocations) and Miri randomized it (like normal allocations).

r? `@oli-obk`
@jieyouxu jieyouxu mentioned this pull request Jul 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library 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.
7 participants
@RalfJung @rustbot @rust-log-analyzer @theemathas @oli-obk @bors @workingjubilee