-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add a Lint for Pointer to Integer Transmutes in Consts #130540
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
What's the motivation for this lint? https://doc.rust-lang.org/std/primitive.pointer.html#method.addr and https://doc.rust-lang.org/std/ptr/fn.without_provenance.html are transmutes, and I'd thought that this was considered basically fine now, albeit possibly not what you wanted if you need to preserve provenance. |
Transmuting a pointer to integer in const context is undefined behavior1. Usually, the evaluator will abort and emit an error when it comes across such undefined transmutes. But const functions and associated consts are evaluated only when referenced. This can result in undefined behavior in a library going unnoticed until the function or constant is actually used. Therefore, this lint specifically targets pointer to integer transmutes in const functions and associated consts. From what I can tell, this lint is not related to the functions you have linked because:
Footnotes |
Oh, of course; thank you. Brain fart on my part. Seem hard to lint on when it's not always UB, because things like I'd be tempted to run it on optimized mir where we can have const-folded a bunch of those cases away already, but then it'd be super inconsistent which we probably don't want either :/ |
Also, we're not optimizing const MIR. |
Indeed, that's the tricky part. The lint triggers in this code which is exactly an example of that: rust/library/core/src/ptr/mod.rs Lines 1989 to 1993 in b5bd0fe
|
c005409
to
1a5c922
Compare
This comment has been minimized.
This comment has been minimized.
1a5c922
to
1fb50a5
Compare
This comment has been minimized.
This comment has been minimized.
1fb50a5
to
7867a29
Compare
This comment has been minimized.
This comment has been minimized.
7867a29
to
44c1bca
Compare
This comment has been minimized.
This comment has been minimized.
44c1bca
to
f4e7d8f
Compare
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
f4e7d8f
to
dd73d4f
Compare
This comment has been minimized.
This comment has been minimized.
dd73d4f
to
191f5e4
Compare
This comment has been minimized.
This comment has been minimized.
191f5e4
to
4257454
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Clippy changes remove a test case that tests that a Clippy lint doesn't trigger for a certain pattern and adds this new test case, that tests a rustc
lint in the Clippy test suite.
I think the better thing to do here would be to keep the test case where it is and allow the new rustc
lint with an outer attribute just on the issue_12402
function. Maybe give the allow attribute a reason = ""
. We still need to test that our other transmute lints do not trigger on this and this new test case doesn't reflect what we want to actually test with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change looks correct, but I have some questions.
Shouldn't we also account for as
? Is align_offset
invoking UB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this request.
No, that function is treated specially by the const interpreter to ensure that the pointer argument is always actually an integer. |
4257454
to
20782e4
Compare
This comment has been minimized.
This comment has been minimized.
20782e4
to
dbce836
Compare
This comment has been minimized.
This comment has been minimized.
dbce836
to
ab86735
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (daebce4): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.1%, secondary 1.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 774.072s -> 774.475s (0.05%) |
Add a Lint for Pointer to Integer Transmutes in Consts Fixes rust-lang#87525 This PR adds a MirLint for pointer to integer transmutes in const functions and associated consts. The implementation closely follows this comment: rust-lang#85769 (comment). More details about the implementation can be found in the comments. Note: This could break some sound code as mentioned by RalfJung in rust-lang#85769 (comment): > ... technically const-code could transmute/cast an int to a ptr and then transmute it back and that would be correct -- so the lint will deny some sound code. Does not seem terribly likely though. References: 1. https://doc.rust-lang.org/std/mem/fn.transmute.html 2. https://doc.rust-lang.org/reference/items/associated-items.html#associated-constants
Fixes #87525
This PR adds a MirLint for pointer to integer transmutes in const functions and associated consts. The implementation closely follows this comment: #85769 (comment). More details about the implementation can be found in the comments.
Note: This could break some sound code as mentioned by RalfJung in #85769 (comment):
References: