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
enable -Zrandomize-layout in debug CI builds #101339
base: master
Are you sure you want to change the base?
Conversation
…layout Additionally teach compiletest to ignore tests that rely on deterministic layout. Tests themselves aren't built with randomization but they can still observe slight changes in std or rustc
(rust-highfive has picked a reviewer for you, use r? to override) |
r? @jyn514 |
1acae66
to
66b5d6c
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.
This seems reasonable, but I'm worried it makes tests more flaky. What's the plan if we merge this and suddenly a quarter of all PRs start failing randomly? I guess "revert until the tests are fixed" is good enough.
I don't see any changes to src/ci
- are there already some builders that enable rust.debug
? Which builders are those? Because this disables tests I want to be careful about where we enable it, we should have at least one builder on each platform with randomized layouts disabled.
if builder.config.rust_randomize_layout { | ||
cmd.arg("--rust-randomized-layout"); | ||
} |
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've been meaning to refactor this for a while, your change only affects compiletest :( there's no one place to put it so it also affects src/test/{codegen,run-make,debuginfo}.
compiletest seems good enough for now, but it would be nice to add this everywhere else if you have time.
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.
That flag is there for the needs-deterministic-layouts
directive. As long as those test types don't get broken by randomization we won't need it.
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.
As long as those test types don't get broken by randomization we won't need it.
well, that's what I'm worried about 😅 but given this is all internal-only, I'm fine with landing this for only a subset of the testsuite.
alternatively we can try and get the two failing UI tests to work with randomized layouts, that would probably be my preference. but won't block on it. |
Ah, good question. x86_64-gnu-debug seems to be the only one enabling That builder only runs run-make-fulldeps so that doesn't get us much test-coverage, but it does make a stage-2 build, so it builds a compiler with a randomized compiler which would hopefully give us some coverage... but not all that much.
If it ever becomes too big of a problem we could set |
That seems fine :) random-layout shouldn't add any additional compile time, and running it in PRs will get us more coverage. It does mean this will only run on linux platforms, but that seems ~fine for now, better than no coverage. |
One prints hir statistics from the compiler and checks them via the test-output. Those statistics involve struct sizes. So layout randomization of the compiler breaks this test quite directly. It's not the structs in the test that get randomized, it's the compiler itself. The other one involves diagnostics that print memory contents of a std type which can get swapped under randomization. |
66b5d6c
to
e4dcd3c
Compare
You can normalize the sizes with a regex: https://rustc-dev-guide.rust-lang.org/tests/ui.html#normalization. I don't think we're directly testing the sizes, just that we output a number.
Hmm, I agree that one's harder. Not sure how to fix it without digging into it myself. |
I guess they might catch struct size-regressions if they're not all of those structs are covered by asserts in the compiler. Ping @nnethercote to clarify. |
e4dcd3c
to
4fee75e
Compare
This comment has been minimized.
This comment has been minimized.
4fee75e
to
0ee6fa7
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
⌛ Testing commit ea09cbd with merge 7613125631f18e024b670e1764b3eeeca257045e... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
I guess that assert didn't account for the possibility of field reordering? But I'm not sure if this isn't a genuine issue. CC @Kixiron |
The respective loop uses |
Filed #101646 with some additional tracing output. |
⌛ Testing commit ea09cbd with merge 34e35a95c7b4e5d60f09f05cfa3853773d89ad18... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Somehow this is still in the queue? @bors r- |
☔ The latest upstream changes (presumably #102051) made this pull request unmergeable. Please resolve the merge conflicts. |
This builds rustc/libs/tools with
-Zrandomize-layout
on *-debug CI runners.Only a handful of tests and asserts break with that enabled, which is promising. One test was fixable, the rest is dealt with by disabling them through new cargo features or compiletest directives.
The config.toml flag
rust.randomize-layout
takes its default fromrust.debug
as overflow checks and debug asserts do too.Edit(@jyn514): this is currently blocked on #101646