Skip to content
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

compiletest ice tracking #122997

Merged
merged 13 commits into from
Apr 15, 2024
Merged

compiletest ice tracking #122997

merged 13 commits into from
Apr 15, 2024

Conversation

matthiaskrgr
Copy link
Member

@matthiaskrgr matthiaskrgr commented Mar 24, 2024

see https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/where.20to.20mass-add.20known.20ices.20.2F.20merging.20glacier.20into.20rust/near/429082963

This will allow us to sunset most of https://github.com/rust-lang/glacier
The rustc ices will be tracked directly inside the rust testsuite
There are a couple of .sh tests remaining that I have not ported over yet.

This adds tests/crashes, a file inside this directory MUST ice, otherwise it is considered test-fail.
This will be used to track ICEs from glacier and the bugtracker.
When someones pr accidentally fixes one of these ICEs, they can move the test from crashes into ui for example.

I also added a new tidy lint that warns when a test inside tests/crashes does not have a //@ known-bug: line

the env var COMPILETEST_VERBOSE_CRASHES can be set to get exit code, stderr and stdout of a crash-test to aid debugging/adding tests.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 24, 2024
@matthiaskrgr matthiaskrgr removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 24, 2024
tests/crashes/122909.rs Outdated Show resolved Hide resolved
@matthiaskrgr matthiaskrgr force-pushed the compiletest_ices branch 2 times, most recently from ce95351 to 7fdf1e3 Compare March 24, 2024 12:51
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible also for you to implement a quick tidy check for all the tests/ui/crashes tests to have a known-bug: directive?

// if a test does not crash, consider it an error
match proc_res.status.code() {
Some(101) => (),
_ => self.fatal("expected ICE"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate this message to tell the dev to give the test a better message and move it to tests/ui/something?

Specifically, I don't want people to just copy over the 122909.rs to tests/ui when they go from fail -> pass.

@@ -0,0 +1,15 @@
//@ compile-flags: -Zpolymorphize=on -Zinline-mir=yes
//@ known-bug: #12345
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//@ known-bug: #12345
//@ known-bug: #122909

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guys this is just a initial test to make sure that compile-flags are working :D

@fmease
Copy link
Member

fmease commented Mar 24, 2024

Could the folder be called ices/ instead if that's still available? I personally don't mind the terminology crashes for ICEs but @oli-obk recently posted a comment on one of the issues you reported which you might remember in which he gave the strong recommendation to abstain from using the term crashes to refer to ICEs since crashes are reserved for actual crashes (stack overflows, OOM kills, other interrupts), cc label I-crash.

@matthiaskrgr
Copy link
Member Author

I don't think we should limit ourselves to only track "internal compiler errors" but also track stack overflows and llvm errors if possible, which is why I chose "crashes".

@rust-log-analyzer

This comment has been minimized.

@matthiaskrgr
Copy link
Member Author

@compiler-errors ^ :)

@matthiaskrgr
Copy link
Member Author

Mhh, so current roablocks:

  1. it seems everything for Crashes is run as check-fail but llvm assertion failures need something like build-fail
  2. it would be beneficial to turn off backtraces for for all crash-tests as that could save 20-30% of runtime for small tests I think.

maybe I can wrap my head around some of that tomorrow

@matthiaskrgr matthiaskrgr force-pushed the compiletest_ices branch 2 times, most recently from a8e19ac to da8597f Compare March 29, 2024 18:49
Comment on lines 368 to 384
// if a test does not crash, consider it an error
match proc_res.status.code() {
Some(101) => (),
_ => self.fatal("expected ICE"),
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soo it turns out that this is not very sufficient at detecting when rustc is killed by a signal for example due to stack overflow.

There is something like std::os::unix::process::ExitStatusExt to get more information for cases like this, but it seems to be unix-only.

I think the best approach for now is to switch around the logic to only "deny" cases where exit status is 0 (no error) or 1 (compiler error) and accept everything else as ICE...?

        if !proc_res.status.success() {
            match proc_res.status.code() {
                Some(1 | 0) => self.fatal(&format!("expected ICE")),
                _ => (),
            }
        }

@matthiaskrgr matthiaskrgr force-pushed the compiletest_ices branch 2 times, most recently from 1c2eca9 to de73263 Compare March 29, 2024 23:43
@matthiaskrgr matthiaskrgr marked this pull request as ready for review March 29, 2024 23:44
@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@matthiaskrgr
Copy link
Member Author

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned compiler-errors Mar 29, 2024
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 14, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 14, 2024
@matthiaskrgr
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Apr 14, 2024

📌 Commit 2ce487c has been approved by oli-obk

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
…-obk

compiletest ice tracking

see https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/where.20to.20mass-add.20known.20ices.20.2F.20merging.20glacier.20into.20rust/near/429082963

This will allow us to sunset most of https://github.com/rust-lang/glacier
The rustc ices will be tracked directly inside the rust testsuite
There are a couple of .sh tests remaining that I have not ported over yet.

This adds `tests/crashes`, a file inside this directory MUST ice, otherwise it is considered test-fail.
This will be used to track ICEs from glacier and the bugtracker.
When someones pr accidentally fixes one of these ICEs, they can move the test from `crashes` into `ui` for example.

I also added a new tidy lint that warns when a test inside `tests/crashes` does not have a `//@ known-bug: ` line

the env var `COMPILETEST_VERBOSE_CRASHES` can be set to get exit code, stderr and stdout of a crash-test to aid debugging/adding tests.
@bors
Copy link
Contributor

bors commented Apr 14, 2024

⌛ Testing commit 2ce487c with merge 22bfeee...

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] rustc_middle test:false 103.135
   Compiling rustc_lint v0.0.0 (/checkout/compiler/rustc_lint)
[RUSTC-TIMING] rustc_resolve test:false 65.818
   Compiling rustc_codegen_llvm v0.0.0 (/checkout/compiler/rustc_codegen_llvm)
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

Session terminated, killing shell...::group::Clock drift check
  network time: Mon, 15 Apr 2024 00:02:00 GMT
##[endgroup]
 ...killed.
##[error]The operation was canceled.

@bors
Copy link
Contributor

bors commented Apr 15, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 15, 2024
@matthiaskrgr
Copy link
Member Author

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2024
@bors
Copy link
Contributor

bors commented Apr 15, 2024

⌛ Testing commit 2ce487c with merge 85b884b...

@bors
Copy link
Contributor

bors commented Apr 15, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 85b884b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 15, 2024
@bors bors merged commit 85b884b into rust-lang:master Apr 15, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 15, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (85b884b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.058s -> 677.003s (-0.01%)
Artifact size: 315.97 MiB -> 316.05 MiB (0.02%)

// if a test does not crash, consider it an error
if proc_res.status.success() || matches!(proc_res.status.code(), Some(1 | 0)) {
self.fatal(&format!(
"test no longer crashes/triggers ICE! Please give it a mearningful name, \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't mind me, I'm just passing by

Suggested change
"test no longer crashes/triggers ICE! Please give it a mearningful name, \
"test no longer crashes/triggers ICE! Please give it a meaningful name, \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think oli already fixed this in a drive-by cleanup somewhere 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, did he? I just saw that in one of this PRs #126316 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember doing that, but it probably hasn't landed yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.