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

UI Tests that check ICEing thread names break with parallel-compiler = true #65047

Closed
anp opened this issue Oct 3, 2019 · 11 comments
Closed
Labels
A-parallel-queries Area: Parallel query execution C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler

Comments

@anp
Copy link
Member

anp commented Oct 3, 2019

When I run ./x.py test locally on a clean checkout of master with parallel-compiler = true, I see a couple of failures that seem to be related to the name of the rustc thread which ICEd in the tests. I turned parallel-compiler off, reran the test command, and the tests passed.

Here's the stderr diffs of the failing tests:

---- [ui] ui/consts/miri_unleashed/mutable_references_ice.rs stdout ----
diff of stderr:

4	LL |     x: &UnsafeCell::new(42),
5	   |        ^^^^^^^^^^^^^^^^^^^^
6	
-	thread 'rustc' panicked at 'assertion failed: `(left != right)`
+	thread '<unnamed>' panicked at 'assertion failed: `(left != right)`
8	  left: `Const`,

...

---- [ui] ui/pattern/const-pat-ice.rs stdout ----
diff of stderr:

-	thread 'rustc' panicked at 'assertion failed: rows.iter().all(|r| r.len() == v.len())', src/librustc_mir/hair/pattern/_match.rs:LL:CC
+	thread '<unnamed>' panicked at 'assertion failed: rows.iter().all(|r| r.len() == v.len())', src/librustc_mir/hair/pattern/_match.rs:LL:CC
2	note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

My assumption is that the parallel-compiler flag doesn't result in all of the threads being named rustc, and so when the ICE occurs it produces an error message from an unnamed thread. I also assume this is totally OK in most cases, because very few things rely on the names of compiler threads.

If these completely unfounded assumptions are accurate, then would it be reasonable to set the name of threads in the compiler's pool?

  • Commit: c6293e3
  • OS: Arch Linux (fairly current as of filing)
@Centril Centril added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2019
@jonas-schievink jonas-schievink added A-parallel-queries Area: Parallel query execution C-bug Category: This is a bug. labels Oct 3, 2019
@pnkfelix pnkfelix added the WG-compiler-parallel Working group: Parallelizing the compiler label Oct 3, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

triage: P-high, since fixing this is presumably high impact for WG-parallel-rustc. Removing nomination label.

@mati865
Copy link
Contributor

mati865 commented Oct 3, 2019

Would it be enough to normalize output of those two tests similarly to this?

// normalize-stderr-test "note: rustc 1.* running on .*" -> "note: rustc VERSION running on TARGET"

@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

(also, I'm tempted to remove the ICE label. Its a little weird that we have tests that are checking the output format of ICEs, but regardless of that, this bug is not about addressing any ICEs in the compiler itself.)

@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

Would it be enough to normalize output of those two tests similarly to this?

// normalize-stderr-test "note: rustc 1.* running on .*" -> "note: rustc VERSION running on TARGET"

Yes I suspect something like that might well suffice.

@pnkfelix pnkfelix added P-high High priority E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed I-nominated labels Oct 3, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

(I'm willing to mentor fixes to this bug. I suspect there are very easy solutions available.)

@hbina
Copy link
Contributor

hbina commented Oct 3, 2019

I will try to reproduce this error, if I can I want to try to take it down!

@hbina
Copy link
Contributor

hbina commented Oct 3, 2019

Tested it on Ubuntu 18.04, and it did panic. Waiting on instructions, I have little clue as how to proceed.

@cuviper
Copy link
Member

cuviper commented Oct 3, 2019

My assumption is that the parallel-compiler flag doesn't result in all of the threads being named rustc, and so when the ICE occurs it produces an error message from an unnamed thread. I also assume this is totally OK in most cases, because very few things rely on the names of compiler threads.

Yes, and I have a commit to add thread names in #64358, 4152abe. We could easily apply that separately without waiting for the new rustc-rayon to be published.

@hbina
Copy link
Contributor

hbina commented Oct 4, 2019

Hmm, so I need to apply the same changes to this issue as well? @cuviper
I might be a bit slow, I borked my Linux machine ...

@cuviper
Copy link
Member

cuviper commented Oct 7, 2019

@hbina Yes, if you apply the same thread_name change, it will fix this issue. I'm also hoping that the wait on new rustc-rayon will be resolved soon, so we can just land my PR in full.

Either way, I don't think this issue deserves P-high -- the actual ICE behavior of these tests is correctly handled, but for the cosmetic thread name.

@pnkfelix
Copy link
Member

closing as fixed since #64358 landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parallel-queries Area: Parallel query execution C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler
Projects
None yet
Development

No branches or pull requests

7 participants