Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Test that fixed ICEs stay fixed #1156

Open
tmandry opened this issue Mar 5, 2022 · 5 comments
Open

Test that fixed ICEs stay fixed #1156

tmandry opened this issue Mar 5, 2022 · 5 comments

Comments

@tmandry
Copy link
Member

tmandry commented Mar 5, 2022

See rust-lang/rust#80706 (comment).

No one noticed when this ICE regressed, and the regression made it into stable. In this case there was a test added in rust-lang/rust but it was not sufficient.

It seems like this project can provide redundancy on testing so we notice when ICEs have regressed in addition to when they are fixed.

@Alexendoo Alexendoo self-assigned this Mar 5, 2022
@matthiaskrgr
Copy link
Member

matthiaskrgr commented Mar 5, 2022

I can see that fixed/80706.rs still crashes.
I think the problem might be that the crash requires --edition 2018 in order to correctly parse the file, glacier does not know that.
When rustc switched from 2018 to 2021 as default edition, we lost the ability to correctly reproduce the crash in that file..?

EDIT: mh, looks like 2018 is actually the default edition for glacier 😅
https://github.com/rust-lang/glacier/blob/master/src/lib.rs#L53

@Alexendoo
Copy link
Member

That one seems to have started ICEing again since rust-lang/rust#89285

@matthiaskrgr
Copy link
Member

I did a quick hack to see how many ICEs there are currently

diff --git a/src/lib.rs b/src/lib.rs
index f6cb24a..952c722 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -7,7 +7,7 @@ use std::{env, fmt};
 pub use rayon;

 static RUSTC: &str = "rustc";
-static ICES_PATH: &str = "ices";
+static ICES_PATH: &str = "fixed";
 static SHELL: &str = "bash";

 #[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
@@ -139,7 +139,10 @@ impl TestResult {

     pub fn outcome_token(&self) -> char {
         match self.outcome {
-            Outcome::ICEd => '.',
+            Outcome::ICEd => {
+                println!("\n{}", self.path().display());
+                '.'
+            }
             Outcome::Errored => 'E',
             Outcome::NoError => 'N',
         }

It it seems that 9 tests inside the fixed directory actually crash:

fixed/72793.rs
fixed/87813.sh
fixed/80229.sh
fixed/80230.sh
fixed/80231.sh
fixed/80706.rs
fixed/23600.rs
fixed/23707.rs
fixed/66696.rs

So apparently glacier just never checks these on its own? 😅

@JohnTitor
Copy link
Member

Yes, it doesn't run on the fixed dir, that dir was just added as per #154. I doubt all of them are valid failures as we don't punish a valid LLVM error, for instance.
So, looking at rust-lang/rust#80706 (comment), the problem is that we did a mistake when adding a regression test on rust-lang/rust. If we've placed the annotation correctly, that test should've prevented the regression. I wouldn't like to do DRY for the regression check, but harden/check it more carefully on reviewing, it would make more sense.

@Alexendoo
Copy link
Member

Oops, I wasn't actually working on this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants