Added randomized field padding to -Z randomize-layout
#97861
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Kixiron
changed the title
-Z randomize-layout
|
This ICEs when I run ICE output |
|
Did you run |
|
Still get the same ICE with an |
bjorn3
reviewed
Jun 9, 2022
| .unwrap_or(0) | ||
| } else { | ||
| 0 | ||
| }); |
There was a problem hiding this comment.
I think there may not be any extra padding for option like types wrapping a type with a guaranteed niche.
There was a problem hiding this comment.
I think you're correct, I just don't know how to make that check
There was a problem hiding this comment.
I could've sworn there was an attribute on Option to indicate it was FFI-friendly (effectively guaranteeing its optimization), but right now it just has #[rustc_diagnostic_item]:
rust/library/core/src/option.rs
Lines 514 to 518 in 083721a
There was a problem hiding this comment.
It has #[rustc_nonnull_optimization_guaranteed], that may help
There was a problem hiding this comment.
Oooh, NonNull does, not Option, hah.
Welp, that's sneaky. Maybe you could pull out the code from SizeSkeleton::compute (or the FFI lint?), to detect Option-like enums, and make it check #[rustc_nonnull_optimization_guaranteed] for a custom type as the only non-ZST field... but I'm not 100% happy with that, I guess we've enshrined Option-like enums more than we should have?
At least such a hack could move things along.
There was a problem hiding this comment.
Do you think it'd break anything to just slap #[rustc_nonnull_optimization_guaranteed] on Option? Or is it possible to check for the Some lang item while calculating the layout for it?
There was a problem hiding this comment.
That would make the ffi safety lint think that Option<Option<&T>> is ffi safe. You should probably copy the logic of repr_nullable_ptr in compiler/rustc_lint/src/types.rs instead. This is what the ffi safety lint uses.
There was a problem hiding this comment.
The meaning is different. #[rustc_nonnull_optimization_guaranteed] struct Foo implies that Option<Foo>, Result<Foo, ()> etc. are all the same size as Foo (well, assuming the niche actually exists, don't think the attribute actually checks for it).
We don't have another attribute for the enum and IIRC the FFI lint allows anything vaguely Option-shaped anyway, so you could mimic that.
If we want attributes on both NonNull and Option, we should find better name for them, as "nonnull payload" and "nullable optimization enum" (not saying that should be in the attribute name, just my first thought about the distinction between the two sides).
cjgillot
added
S-waiting-on-author
eddyb
reviewed
Jun 13, 2022
Comment on lines
1444
to
1445
| max_layout_multiple: Option<u64> = (None, parse_opt_number, [TRACKED], | ||
| "set the maximum layout padding multiple when using 'randomize-layout' (default: 3)"), |
There was a problem hiding this comment.
I don't think -Z max-layout-multiple is clear enough that it's only about alignment padding, not the whole size. I don't have good suggestions but -Z layout-random-padding-max-factor is one of the shortest I can think of, that is self-explanatory to some extent.
|
cc @rust-lang/compiler @rust-lang/miri I'd like more eyes on this (but mainly I want to avoid this feature being mainly discussed privately, with most users largely unaware that they have this tool at their disposal, like what happened with |
|
I believe that @saethlin has already been doing crater-style experiments with |
|
Heh, saw your comment appear while I'm trying to choose my words here. FWIW I've done some study with the previous https://github.com/phsym/prettytable-rs/blob/cf7dca33e2e2131c20e4bbc34ad222c5e6edc29a/src/lib.rs#L514-L517 It would definitely be good to have an accurate survey of the use of this, but it's unclear to me how actionable that would be. A crater run with If such a crater run is done it should go from nightly + -Zbuild-std to nightly + -Zbuild-std + -Zrandomize-layout. Just -Zbuild-std on its own causes quite a few crates to SIGILL due to UB caught by debug assertions in the standard library, which are compiled out by default. |
|
I can't really help review this, but would be happy to see this feature advertised in the Miri README. |
Kixiron
commented
Jun 13, 2022
compiler/rustc_middle/src/ty/util.rs
Outdated
| /// | ||
| /// Currently supports function pointers (`fn(...) -> ..`), `Box<_>`, references, `NonNull<_>`, | ||
| /// `NonZero*` and `#[repr(transparent)]` newtypes around them | ||
| pub fn nonnull_opt_applies<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, is_definition: bool) -> bool { |
There was a problem hiding this comment.
This is somehow slightly incorrect, it looks like previously a true result was being treated as a "maybe" instead of a canonical "yes, this is can be null pointer opt'ed"
Specifically, it fails in std/src/io/error/repr_bitpacked.rs on a size assertion that Option<Repr> is 8 bytes where Repr is
#[repr(transparent)]
struct Repr(NonNull<()>, PhantomData<ErrorData<Box<Custom>>>);Which confuses me since it should work?
nonnull_opt_applies()gets anOption<Repr>and sees thatOptionisOption-shaped- It gets the inner type,
Repr - It passes
Reprtoty_is_known_nonnull()ty_is_known_nonnull()sees thatRepris a#[transparent]ADT which is not a unionReprdoesn't have#[rustc_nonnull_optimization_guaranteed]but that's ok, we continueReprdoesn't haverepr(no_niche)but that's ok, we continue- We find the non-zst field of
Reprwhich isNonNull<()>- We call
ty_is_known_nonnull()onNonNull<()> NonNull<()>is a transparent ADT which isn't a unionNonNull<()>has#[nonnull_optimization_guaranteed], we return true
- We call
.any(|field| ty_is_known_nonnull(field))gets atruefromNonNull<()>and returnstrue
ty_is_known_nonnull(Repr)returnstrue
There was a problem hiding this comment.
Ah, it seems I forgot a ! when calling it
|
So I think I've gotten it working properly, however it ices when compiling BacktraceEdit: This may just be another overzealous check Edit 2: Actually the check is fine, I just need to figure out how to properly handle situations where |
|
@Kixiron Looks like the layout is invalid: you can't have field offsets (or, well, My best guess is the niche optimization assumes that only the "dataful" variant can contribute to the size of the |
|
New day, new ICE I fixed the field offset thing, I was saving the offset value before I was calculating the extra padding, whoops Backtrace |
This comment has been minimized.
This comment has been minimized.
|
ICE from #97861 (comment) is: That's miri (I wish the crate wasn't named Oh I see, it's the checked arithmetic MIR operators, which return cc @oli-obk @RalfJung Could miri easily treat the |
We may end up getting into whack-a-mole territory by just fixing checked arithmethic, there are a lot of things like wide pointers that are pretty much hardcoded to be pairs. Maybe |
That's a different kind of pair - wide pointers will continue to be AFAIK checked arithmetic is a rare case of tuple types being used implicitly in MIR. |
Not easily, no. In fact checked arithmetic is one of the primary reasons why Miri has intermediate scalar pairs to begin with. I would strongly prefer to keep it that way. Scalar pairs can have padding, so why does randomize-layout make them general aggregates here? |
|
In order to keep our performance in regular const eval, we could just make a decision based on whether |
|
Oh, leading padding. Yeah that's annoying.
I could imagine that just something specific for checked arithmetic might actually work fairly reasonably. At least I have an idea.^^ I'll put it on my list. |
To be clear, scalar pairs must have the exact layout of a Now we could extend this, as unlike One might imagine even something like an (bonus: this would also work for e.g. EDIT: I might try playing around with this, or at least going for the "explicit offsets", might even be an optimization over recomputing them all the time. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #99983) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So this feels like it should be working (or at least there's no problems I'm immediately aware of) but I'm getting some llvm bitcode errors Build Log |
This comment has been minimized.
This comment has been minimized.
|
Enabling LLVM asserts, disabling LTO and only building with 1 job gets me this |
|
So, what exactly is the issue going on here, I'm not really sure how to diagnose this |
|
It's failing in llvm when compiling alloc. Not an expert in that area, my guess is that rustc is now emitting invalid LLVM IR. Since it's about pointer casts maybe a pointer is being cast to or from a type that now has padding but shouldn't. The dev guide has a section on LLVM debugging: https://rustc-dev-guide.rust-lang.org/backend/debugging.html |
|
Does this add padding to abi::ScalarPair or abi::Vector? Both have hard coded assumptions about the layout in codegen backends. |
|
I'm not entirely sure, to be honest. There's a separate |
|
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
3 tasks
This comment was marked as off-topic.
This comment was marked as off-topic.
rustbot
removed
the
T-bootstrap
workingjubilee
added
the
-Zrandomize-layout
|
@Kixiron any updates on this? |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds randomized padding before each struct field in valid structs when
-Z randomize-layoutis enabled, the maximum amount of padding is decided by the-Z max-layout-multiple=<MULTIPLE>flag.This allows easier and more reliable detection of layout-based UB by greatly increasing the possibility of different structs having massively different layouts due to their sizes, field offsets, etc. being so different.
This actually introduces one small change in behavior vs. the past
-Z randomize-layout, nowrepr(transparent)structs are never considered for layout randomizationcc @eddyb @saethlin