-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Stop applying dereferenceable(n) to return types
#149948
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
Conversation
| // potentially self-referential types (see | ||
| // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>). If LLVM had a way | ||
| // to say "dereferenceable on entry" we could use it here. | ||
| attrs.pointee_size = match kind { |
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.
drive-by: I really dislike how we track dereferenceable as pointee_size: Size, this should imo really be dereferenceable: Option<NonZeroU64>, or something.
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.
Yeah this entire code could benefit from a large-scale refactor.^^
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.
I might or might not do something about it ^^"
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.
Could probably make NonZero<Size> work, now?
Size is kinda nice when it's coming from layout as Size anyway, and avoids the "type_ix is bits but this is bytes" problems.
(Also, IIRC we don't put dereferencable(0) on &Zst anyway, so using zero instead of None isn't horrible here.)
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.
LLVM doesn't allow dereferencable(0).
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.
Could probably make
NonZero<Size>work, now?
NonZero has a ZeroablePrimitive bound and that trait is sealed, so you can't really make NonZero<Size>, only make your own NonZeroSize...
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6e7dd2c (parent) -> 61cc47e (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 61cc47e367d7be91e13bcd01e4e96e0e233d4f6d --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (61cc47e): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 478.748s -> 478.546s (-0.04%) |
It looks like the semantics of
dereferenceable(n)on return types is "dereferenceable until the end of the program", which is not sound for how we were using it. See dereferenceable on return type zulip thread.cc @rust-lang/opsem @nikic