Enable a weaker form of -Zrandomize-layout when using debug-assertions #139719
+112
−35
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
A-attributes
|
The job Click to see the possible cause of the failure (guessed by this bot) |
scottmcm
reviewed
Apr 12, 2025
Comment on lines
+1491
to
+1494
| // Never randomize layout if we're not running under debug assertions. | ||
| if !self.sess.opts.debug_assertions { | ||
| flags.insert(ReprFlags::NEVER_RANDOMIZE_LAYOUT); | ||
| } |
There was a problem hiding this comment.
Unsure: should we have a -Z flag for always/never/if-debug-assertions so that this can be exercised independently? Might be nice to have the way to just turn it off broadly without needing to update the type, like for debugging if something breaks to see if it's because of this.
There was a problem hiding this comment.
Hm...
Probably, yeah. I don't think it could use the existing -Zrandomize-layout flag since this is a different form of layout randomization. But yeah, a -Zrandomize-layout-weak=true/false/if-debug-assertions (or just making unspecified mean if-debug-assertions) flag would probably be the right move.
There was a problem hiding this comment.
or just making unspecified mean if-debug-assertions
Yeah, that's what I was thinking -- use the opt_bool parser and leave it defaulting to exactly what you have here, but have the way for people to force it to one or the other if they really need to.
(Could even then always pass that when compiling library, if it came down to it too, though that's just mentioned as musing. I haven't thought about whether it's actually a good idea or not, and it might be terrible.)
scottmcm
reviewed
Apr 12, 2025
| @@ -1,6 +1,7 @@ | |||
| //@ compile-flags: -Copt-level=3 | |||
| //@ revisions: host x86-64-v3 | |||
| //@ min-llvm-version: 20 | |||
| //@ ignore-std-debug-assertions | |||
There was a problem hiding this comment.
...that would also let tests like this just pass a -Z flag instead of not running.
There was a problem hiding this comment.
Do codegen tests have the ability to rebuild std? I assume it's some type in std that's being randomized and causing the worse codegen. When I tried disabling debug assertions, it didn't seem to change the codegen at all.
There was a problem hiding this comment.
Hmm, this one might have been a bad one to put that comment on.
That said, I'm surprised you're not getting a conflict on this file, 'cause I just updated it yesterday https://github.com/rust-lang/rust/pull/139430/files#diff-a6f7809c5adbbdcc5080d91b5615b2d7ec2e69a5df97470dd3d7d13a9a90cab0R2
(It's been flakey lately in general, so we ended up just disabling the v3 part of it, so if that was the problem you were hitting, it might be already fixed on rebase.)
There was a problem hiding this comment.
Do codegen tests have the ability to rebuild std?
They do not.
scottmcm
reviewed
Apr 13, 2025
Comment on lines
+1183
to
+1184
| // Shuffle the ordering of the fields. | ||
| optimizing.shuffle(&mut rng); |
There was a problem hiding this comment.
👍, I like this general approach. We saw in crater that enough places have const asserts on sizes that trying to run with full arbitrary randomization just didn't work, but just shuffling first then still running the layout optimization should get us a meaningful part of the checking. (Though with niche awareness and such now too, there's rather alot of types that it just can't touch, so I don't know if it's even a majority of types where the shuffling will really end up permuting things.)
|
@rustbot author (since CI is failing) |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
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.
Related to (but doesn't directly use) #106764.
Going to r? @scottmcm
to look at this first since we were discussing this.
I have no idea why the
tests/ui/print_type_sizes/async.stdouthas changed and now has padding? The overall size didn't change though.