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

Reevaluate Miri engine alignment checks #63085

Closed
RalfJung opened this issue Jul 28, 2019 · 2 comments · Fixed by #98846
Closed

Reevaluate Miri engine alignment checks #63085

RalfJung opened this issue Jul 28, 2019 · 2 comments · Fixed by #98846
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Two PRs will likely land soon that change the way the Miri engine does alignment checks: #63079 and #63075.

The former entirely disables alignment checks for CTFE (when the Miri engine is used as part of compilation). The latter moves the checking of "access validity" of a pointer (non-dangling and aligned) to the moment the pointer is dereferenced (using the * operator), as opposed to doing the checks only when a load/store actually happens.

As a consequence, the align field of MemPlace is now effectively dead weight in both instances of the Miri engine. However, Miri might need it again in the future if we decide to relax the rules for dereferencing pointers, which is a discussion I want to have after rust-lang/rfcs#2582. Then we should either remove the align field for good, or try to abstract it through the Machine trait so that CTFE does not have to carry it. Also see #63079 (comment).

Cc @oli-obk

@jonas-schievink jonas-schievink added A-miri Area: The miri tool T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 28, 2019
@RalfJung RalfJung added the A-const-eval Area: Constant evaluation (MIR interpretation) label Jul 28, 2019
@RalfJung
Copy link
Member Author

@digama0 points out that an alternative future might be for the align field to move to PlaceTy, treating it conceptually more like part of the type of this place than its dynamic value. This would be good enough for what alignment is currently used for (justifying the alignment annotations on loads/stores); the sublte key difference lies in whether the dynamic alignment of trailing fields of unsized structs is factored in or not. (Currently it is.)

@RalfJung
Copy link
Member Author

RalfJung commented Jun 8, 2022

@digama0 points out that an alternative future might be for the align field to move to PlaceTy, treating it conceptually more like part of the type of this place than its dynamic value.

FWIW, that is exactly how I am treating place alignment in MiniRust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants