Try to ensure usize marker does not get merged #69651
Conversation
rust-highfive
added
the
S-waiting-on-review
| // `usize` as it's first argument, ensuring correctness below when we read | ||
| // out the associated value from `ArgumentV1`. | ||
| let _v: usize = *crate::hint::black_box(ptr); | ||
| loop {} |
There was a problem hiding this comment.
If you make this return Ok(()), you could actually call it before reading the usize from Argument, ensuring that what this closure does is guaranteed to happen, giving more confidence that if there was UB, it was invoked by calling the fn pointer which should be safe to call.
There was a problem hiding this comment.
It's a bit annoying to call the formatter as we don't have a &mut Formatter<'_> around and I'd rather not manufacture one just for this. I think we could likely thread it through but I think that hurts readability and such so I'd rather not.
There was a problem hiding this comment.
Oh, right, that method doesn't take the formatter, fair enough.
Mark-Simulacrum
force-pushed
the
black-box-marker
branch
from
976aff7
to
03e4bb2
Compare
|
Changed to |
src/libcore/fmt/mod.rs
Outdated
| // *do* get collapsed with some other function, that function also takes a | ||
| // `usize` as it's first argument, ensuring correctness below when we read | ||
| // out the associated value from `ArgumentV1`. | ||
| let _v: usize = crate::hint::black_box(*ptr); |
There was a problem hiding this comment.
Are we using black_box here for functional correctness?
There was a problem hiding this comment.
Yes, we were, I think I agree we probably shouldn't be. Switched to a volatile read.
Mark-Simulacrum
force-pushed
the
black-box-marker
branch
from
03e4bb2
to
a9259fb
Compare
|
@bors r+ |
|
📌 Commit a9259fb has been approved by |
bors
added
S-waiting-on-bors
…=eddyb Try to ensure usize marker does not get merged This follows up on [this conversation](rust-lang#69209 (comment)). However, I'm not confident this is quite correct, so feedback is appreciated, as always.
Rollup of 8 pull requests Successful merges: - #69631 (remove non-sysroot sources from rust-src component) - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests)) - #69651 (Try to ensure usize marker does not get merged) - #69668 (More documentation and simplification of BTreeMap's internals) - #69685 (unix: Don't override existing SIGSEGV/BUS handlers) - #69771 (Cleanup E0390 explanation) - #69777 (Add missing ` in doc for File::with_options()) - #69812 (Refactorings to method/probe.rs and CrateId) Failed merges: r? @ghost
Rollup of 7 pull requests Successful merges: - #69631 (remove non-sysroot sources from rust-src component) - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests)) - #69651 (Try to ensure usize marker does not get merged) - #69668 (More documentation and simplification of BTreeMap's internals) - #69771 (Cleanup E0390 explanation) - #69777 (Add missing ` in doc for File::with_options()) - #69812 (Refactorings to method/probe.rs and CrateId) Failed merges: r? @ghost
This follows up on this conversation. However, I'm not confident this is quite correct, so feedback is appreciated, as always.