enable -Zrandomize-layout in debug CI builds by the8472 · Pull Request #101339 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation68 Commits7 Checks6 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

the8472

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 defaults to false, so it has to be explicitly enabled for now.

@rust-highfive

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Sep 2, 2022

@the8472

jyn514

Member

@jyn514 jyn514 left a comment • Loading

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.

Comment on lines 1446 to 1448

if builder.config.rust_randomize_layout {
cmd.arg("--rust-randomized-layout");
}

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.

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.

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.

@jyn514 jyn514 added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

A-testsuite

Area: The testsuite used to check the correctness of rustc

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 2, 2022

@jyn514

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.

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.

@the8472

I don't see any changes to src/ci - are there already some builders that enable rust.debug?

Ah, good question. x86_64-gnu-debug seems to be the only one enabling rust.debug. Other CI jobs only toggle some of the more specific flags in a pattern that's not entirely clear to me.

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.
Maybe enable it for the llvm-13 builder? That even runs on PRs.

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?

If it ever becomes too big of a problem we could set -Zlayout-seed to use a fixed randomization seed. Until that happens it seems better to stochastically cover more bases.

@jyn514

Maybe enable it for the llvm-13 builder? That even runs on PRs.

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.

@the8472

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.

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.

@rustbot rustbot added the T-infra

Relevant to the infrastructure team, which will review and decide on the PR/issue.

label

Sep 2, 2022

@jyn514

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.

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.

The other one involves diagnostics that print memory contents of a std type which can get swapped under randomization.

Hmm, I agree that one's harder. Not sure how to fix it without digging into it myself.

jyn514

bjorn3

@the8472

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.

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.

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nnethercote

If you are talking about src/test/ui/stats/hir-stats.rs, it exists to test the output of -Zhir-stats. Using regex normalization to hide the exact numbers would weaken the test somewhat, though I guess it would be acceptable. But it's possible that randomization might also change the order of the entries, which would be harder to normalize for.

@bors bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Aug 31, 2024

@the8472

…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

@the8472

@the8472

@the8472

@the8472

@the8472

@the8472

@the8472

@bors r=Mark-Simulacrum

I have left it as disabled-by-default and updated the config.toml.example file to reflect that. Only the x86_64-gnu-llvm-17 runner or explicit opt-in is required for now. This can still be changed later to inherit from rust.debug.

@bors

📌 Commit c218c75 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 31, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 2, 2024

@matthiaskrgr

…-Simulacrum

enable -Zrandomize-layout in debug CI builds

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 defaults to false, so it has to be explicitly enabled for now.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 3, 2024

@bors

…iaskrgr

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

tgross35 added a commit to tgross35/rust that referenced this pull request

Sep 4, 2024

@tgross35

…-Simulacrum

enable -Zrandomize-layout in debug CI builds

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 defaults to false, so it has to be explicitly enabled for now.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 4, 2024

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 4, 2024

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 4, 2024

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@bors

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@rust-timer

Rollup merge of rust-lang#101339 - the8472:ci-randomize-debug, r=Mark-Simulacrum

enable -Zrandomize-layout in debug CI builds

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 defaults to false, so it has to be explicitly enabled for now.

Labels

-Zrandomize-layout

Unstable option: Randomize the layout of types.

A-testsuite

Area: The testsuite used to check the correctness of rustc

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-infra

Relevant to the infrastructure team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.