Miscellaneous cleanup to formatting #69209
Conversation
The contents were always UTF-8 anyway, and &str has an equivalent representation to &[u8], so this should not affect performance while removing unsafety at edges. It may be worth exploring a further adjustment that stores a single byte (instead of 16) as the contents are always "", "-", or "+".
rust-highfive
added
the
S-waiting-on-review
| // containing data (as a matter of static generation of the formatting | ||
| // arguments), so this is merely an additional check. | ||
| #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] | ||
| static USIZE_MARKER: fn(&usize, &mut Formatter<'_>) -> Result = |_, _| loop {}; |
There was a problem hiding this comment.
cc @eddyb -- am I correct that this reasoning is correct / this should work?
There was a problem hiding this comment.
I don't see how it's much more different from the previous approach.
LLVM will replace loads from USIZE_MARKER with the fn pointer, which is still unnamed_addr.
There was a problem hiding this comment.
In my reading of your comment in a different issue, my understanding is that the non-determinism is strictly in the casting from a function (or closure) to the function pointer.
But even if the closure here is unnamed_addr, the function pointer produced is guaranteed to be stable. I guess what we don't guarantee is that this closure did not get merged with a different function. But that's fine, we don't technically need to -- anywhere that this could have gotten merged, would have to be associated with a usize in the "value" part of Argument, right? So the safety property would still hold.
There was a problem hiding this comment.
Ah, I see what the argument is. Indeed, we shouldn't introduce non-determinism and USIZE_MARKER == USIZE_MARKER should always hold.
The funny thing though is that because you used |_, _| loop {}, anyone doing the same thing (loop {} in the body) could end up being collapsed to this (I don't think the signature matters, because usize is not passed by value).
There was a problem hiding this comment.
The signature not mattering is an interesting point. There's probably no way to prevent that collapsing, though, right? We could plausibly do something weird (e.g., print the address of the passed references shifted to the right or something) and just say "no one else will do this", I guess.
There was a problem hiding this comment.
I think you should at least load an usize and blackbox it.
Otherwise there is no guarantee there is an usize (or a same-size integer) behind the opaque pointer.
There was a problem hiding this comment.
I will try to do this in a follow-up PR to avoid getting this bogged down (and r? you on that one).
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
strip-unsafe
branch
from
970fb38 to
a668adc
Compare
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
strip-unsafe
branch
2 times, most recently
from
4dacfde to
ea29a40
Compare
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
strip-unsafe
branch
from
ea29a40 to
8227b5c
Compare
This comment has been minimized.
This comment has been minimized.
This prevents accidental dereferences and so forth of the Void type, as well as cleaning up the error message to reference Opaque rather than the more complicated PhantomData type.
Currently, function items are always tagged unnamed_addr, which means that casting a function to a function pointer is not guaranteed to produce a deterministic address. However, once a function pointer is created, we do expect that to remain stable. So, this changes the show_usize function to a static containing a function pointer and uses that for comparisons. Notably, a *static* may have 'unstable' address, but the function pointer within it must be constant. Resolves issue 58320.
Mark-Simulacrum
force-pushed
the
strip-unsafe
branch
from
8227b5c to
f6bfdc9
Compare
|
@bors r+ |
|
📌 Commit f6bfdc9 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - #68712 (Add methods to 'leak' RefCell borrows as references with the lifetime of the original reference) - #69209 (Miscellaneous cleanup to formatting) - #69381 (Allow getting `no_std` from the config file) - #69434 (rustc_metadata: Use binary search from standard library) - #69447 (Minor refactoring of statement parsing) Failed merges: r? @ghost
…=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.
…=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.
| extern "C" { | ||
| type Opaque; |
There was a problem hiding this comment.
What's funny is that we merged the PR that adds doc comments with it still using Void (@anyska went to rename Void -> Opaque and found it just in comments, PR coming soon).
Also EDIT: ah no, "C" is redundant, I just realized that now.rustfmt adds it.
Each commit stands alone.
This pull request will also resolve #58320.