-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
don't UB on dangling ptr deref, instead check inbounds on projections #114330
Conversation
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri |
let slice: *const [u8] = mem::transmute((1usize, usize::MAX)); | ||
let _val = &*slice; //~ ERROR: evaluation of constant value failed | ||
//~| slice is bigger than largest supported object | ||
} }; |
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.
This test no longer errors since *
no longer checks that the place is dereferenceable (and CTFE doesn't do validity checks that would catch the invalid reference).
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 am unsure about not testing this behaviour. On the one hand, we'd be testing UB not being detected, on the other hand, we'd want to know if this behaviour changes again...
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 can add it back as a -Zextra-const-ub-checks
test.
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 guess I meant leaving it in as a normal test that now passes, with a note that it could stop passing at any time and that's not a breaking change. But -Zextra-const-ub-checks
is also good.
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.
This also checks that the error is not reported without the flag, so we will notice if behavior changes again.
9a54fd3
to
dff61cb
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit dff61cb7c2f8f2d12ce99373de4e7632f22b603b with merge 930f5dd6fbaa5068ed93e39fac128cc5f4118778... |
This comment has been minimized.
This comment has been minimized.
dff61cb
to
c5179d8
Compare
@bors try |
⌛ Trying commit c5179d863cccdbdf224817e551ab941269e25068 with merge db3fc710c57da8e3db165b63e17f5397b020e4d0... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
c5179d8
to
34bc4f5
Compare
Finished benchmarking commit (db3fc710c57da8e3db165b63e17f5397b020e4d0): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never 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)ResultsThis 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.
CyclesResultsThis 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: missing data |
34bc4f5
to
9555171
Compare
So the stress test actually got faster, but a bunch of other benchmarks get slower. I thought of a way to keep lazy iteration, let's see if that helps. If not, the regression is probably truly caused by doing an inbounds check for each element (rather than a single check upfront), and that will be harder to avoid. @bors try @rust-timer queue |
1c8bbea
to
28b0c87
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.
r=me with or without test re-added
let slice: *const [u8] = mem::transmute((1usize, usize::MAX)); | ||
let _val = &*slice; //~ ERROR: evaluation of constant value failed | ||
//~| slice is bigger than largest supported object | ||
} }; |
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 am unsure about not testing this behaviour. On the one hand, we'd be testing UB not being detected, on the other hand, we'd want to know if this behaviour changes again...
@bors r=oli-obk |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e7bdc5f): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @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)ResultsThis 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.
CyclesResultsThis 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: 624.367s -> 625.899s (0.25%) |
Some regressions were expected base on prior benchmarks, this is as good as I managed to get it. Only secondary benchmarks are affected. The likely cause is that we now are doing inbounds checks on each place projection. Previously we didn't have to do that, the check performed on |
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril * rust-lang/rust#116196 * rust-lang/rust#116824 * rust-lang/rust#116822 * rust-lang/rust#116477 * rust-lang/rust#116826 * rust-lang/rust#116820 * rust-lang/rust#116811 * rust-lang/rust#116808 * rust-lang/rust#116805 * rust-lang/rust#116800 * rust-lang/rust#116798 * rust-lang/rust#116754 * rust-lang/rust#114370 * rust-lang/rust#116804 * rust-lang/rust#116802 * rust-lang/rust#116790 * rust-lang/rust#116786 * rust-lang/rust#116709 * rust-lang/rust#116430 * rust-lang/rust#116257 * rust-lang/rust#114157 * rust-lang/rust#116731 * rust-lang/rust#116550 * rust-lang/rust#114330 * rust-lang/rust#116724 * rust-lang/rust#116782 * rust-lang/rust#116776 * rust-lang/rust#115955 * rust-lang/rust#115196 * rust-lang/rust#116775 * rust-lang/rust#114589 * rust-lang/rust#113747 * rust-lang/rust#116772 * rust-lang/rust#116771 * rust-lang/rust#116760 * rust-lang/rust#116755 * rust-lang/rust#116732 * rust-lang/rust#116522 * rust-lang/rust#116341 * rust-lang/rust#116172 * rust-lang/rust#110604 * rust-lang/rust#110729 * rust-lang/rust#116527 * rust-lang/rust#116688 * rust-lang/rust#116757 * rust-lang/rust#116753 * rust-lang/rust#116748 * rust-lang/rust#116741 * rust-lang/rust#116594 * rust-lang/rust#116691 * rust-lang/rust#116643 * rust-lang/rust#116683 * rust-lang/rust#116635 * rust-lang/rust#115515 * rust-lang/rust#116742 * rust-lang/rust#116661 * rust-lang/rust#116576 * rust-lang/rust#116540 * rust-lang/rust#116352 * rust-lang/rust#116737 * rust-lang/rust#116730 * rust-lang/rust#116723 * rust-lang/rust#116715 * rust-lang/rust#116603 * rust-lang/rust#116591 * rust-lang/rust#115439 * rust-lang/rust#116264 * rust-lang/rust#116727 * rust-lang/rust#116704 * rust-lang/rust#116696 * rust-lang/rust#116695 * rust-lang/rust#116644 * rust-lang/rust#116630 * rust-lang/rust#116728 * rust-lang/rust#116689 * rust-lang/rust#116679 * rust-lang/rust#116618 * rust-lang/rust#116577 * rust-lang/rust#115653 * rust-lang/rust#116702 * rust-lang/rust#116015 * rust-lang/rust#115822 * rust-lang/rust#116407 * rust-lang/rust#115719 * rust-lang/rust#115524 * rust-lang/rust#116705 * rust-lang/rust#116645 * rust-lang/rust#116233 * rust-lang/rust#115108 * rust-lang/rust#116670 * rust-lang/rust#116676 * rust-lang/rust#116666 Co-authored-by: Benoît du Garreau <bdgdlm@outlook.com> Co-authored-by: Colin Finck <colin@reactos.org> Co-authored-by: Ian Jackson <ijackson@chiark.greenend.org.uk> Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com> Co-authored-by: León Orell Valerian Liehr <me@fmease.dev> Co-authored-by: Trevor Gross <tmgross@umich.edu> Co-authored-by: Evan Merlock <evan@merlock.dev> Co-authored-by: joboet <jonasboettiger@icloud.com> Co-authored-by: Ralf Jung <post@ralfj.de> Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com> Co-authored-by: onur-ozkan <work@onurozkan.dev> Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com> Co-authored-by: The 8472 <git@infinite-source.de> Co-authored-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Co-authored-by: reez12g <reez12g@gmail.com> Co-authored-by: Jakub Beránek <berykubik@gmail.com>
Visiting for weekly perf triage
@rustbot label: +perf-regression-triaged |
…errors fix comment on PlaceMention semantics It seems this was simply missed in rust-lang#114330.
Rollup merge of rust-lang#129355 - RalfJung:PlaceMention, r=compiler-errors fix comment on PlaceMention semantics It seems this was simply missed in rust-lang#114330.
This implements rust-lang/reference#1387 in Miri. See that PR for what the change is about.
Detecting dangling references in
let x = &...;
is now done by validity checking only, so some tests need to have validity checking enabled. There is no longer inherently a "nodangle" check in evaluating the expression&*ptr
(aside from the aliasing model).r? @oli-obk
Based on: