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

Fix problems with backtraces in two ui tests. #111052

Merged
merged 2 commits into from
May 5, 2023

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented May 1, 2023

default-backtrace-ice.rs started started failing for me recently,
because on my Ubuntu 23.04 system there are 100 stack frames, and the
current stack filtering pattern doesn't match on a stack frame with a
three digit number.

issue-86800.rs can also be improved, backtrace-wise.

r? @Nilstrieb

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 1, 2023
@nnethercote
Copy link
Contributor Author

@bors rollup

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nnethercote nnethercote changed the title Fix failing tests/ui/panics/default-backtrace-ice.rs test. Don't print backtrace on ICEs in ui tests. May 1, 2023
@nnethercote
Copy link
Contributor Author

After some discussion with @ehuss:

  • I think the change to issue-86800.rs is fine. That's just a vanilla panic, it doesn't need the backtrace.
  • default-backtrace-ice.rs is trickier. It was introduced in Make rustc use RUST_BACKTRACE=full by default #93566 by @Aaron1011. It's meant to test that an ICE triggers a RUST_BACKTRACE=full backtrace rather than a RUST_BACKTRACE=1 backtrace. But, I don't think it actually does that. The only difference between the full and 1 cases is how many stack frames are printed. And the existing filtering in that test filters out all stack frames. So it can't actually distinguish full and 1, AFAICT. If there was a different label at the top (e.g. "full stack backtrace" vs. "stack backtrace") then it could distinguish.

@nnethercote nnethercote changed the title Don't print backtrace on ICEs in ui tests. Fix problems with backtraces in two ui tests. May 2, 2023
@rustbot rustbot assigned Noratrieb and unassigned Aaron1011 May 2, 2023
@nnethercote
Copy link
Contributor Author

Ok, I found a way to change default-backtrace-ice.rs that (a) fixes the failure I see on my machine, and (b) actually tests that the backtrace is full. Yay!

@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2023

📌 Commit 5f45c69 has been approved by Nilstrieb

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 May 2, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
Fix problems with backtraces in two ui tests.

`default-backtrace-ice.rs` started started failing for me recently,
because on my Ubuntu 23.04 system there are 100 stack frames, and the
current stack filtering pattern doesn't match on a stack frame with a
three digit number.

`issue-86800.rs` can also be improved, backtrace-wise.

r? `@Nilstrieb`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
Fix problems with backtraces in two ui tests.

`default-backtrace-ice.rs` started started failing for me recently,
because on my Ubuntu 23.04 system there are 100 stack frames, and the
current stack filtering pattern doesn't match on a stack frame with a
three digit number.

`issue-86800.rs` can also be improved, backtrace-wise.

r? ``@Nilstrieb``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
Fix problems with backtraces in two ui tests.

`default-backtrace-ice.rs` started started failing for me recently,
because on my Ubuntu 23.04 system there are 100 stack frames, and the
current stack filtering pattern doesn't match on a stack frame with a
three digit number.

`issue-86800.rs` can also be improved, backtrace-wise.

r? ```@Nilstrieb```
Manishearth added a commit to Manishearth/rust that referenced this pull request May 4, 2023
Fix problems with backtraces in two ui tests.

`default-backtrace-ice.rs` started started failing for me recently,
because on my Ubuntu 23.04 system there are 100 stack frames, and the
current stack filtering pattern doesn't match on a stack frame with a
three digit number.

`issue-86800.rs` can also be improved, backtrace-wise.

r? ````@Nilstrieb````
@Manishearth
Copy link
Member

@bors r-

#111204 (comment)

@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 May 4, 2023
Because it then just has to be filtered out.

This change makes this test more like these other tests:
- tests/ui/treat-err-as-bug/err.rs
- tests/ui/treat-err-as-bug/delay_span_bug.rs
- tests/ui/mir/validate/storage-live.rs
- tests/ui/associated-inherent-types/bugs/ice-substitution.rs
- tests/ui/layout/valid_range_oob.rs
This test is supposed to ensure that full backtraces are used for ICEs.
But it doesn't actually do that -- the filtering done cannot distinguish
between a full backtrace versus a short backtrace.

So this commit changes the filtering to preserve the existence of
`__rust_{begin,end}_short_backtrace` markers, which only appear in full
backtraces. This change means the test now tests what it is supposed to
test.

Also, the existing filtering included a rule that excluded any line
starting with two spaces. This was too strong because it filtered out
some parts of the error message. (This was not a showstopper). It was
also not strong enough because it didn't work with three digit stack
frame numbers, which just started seeing after upgrading my Ubuntu
distro to 23.04 machine (this *was* a showstopper).

So the commit replaces that rule with two more precise rules, one for
lines with stack frame numbers, and one for "at ..." lines.
@nnethercote
Copy link
Contributor Author

Tests for stack traces are always fun like this. I've tried ignoring msvc platforms.

@bors try

@bors
Copy link
Contributor

bors commented May 4, 2023

⌛ Trying commit f20738d with merge b35082701814d07b4b0fefce3753957fe244de7b...

@Manishearth
Copy link
Member

r=me on the fixups, you can bors r=Nilstrieb when ready

@bors delegate+

@bors
Copy link
Contributor

bors commented May 4, 2023

✌️ @nnethercote can now approve this pull request

@bors
Copy link
Contributor

bors commented May 4, 2023

☀️ Try build successful - checks-actions
Build commit: b35082701814d07b4b0fefce3753957fe244de7b (b35082701814d07b4b0fefce3753957fe244de7b)

@Manishearth
Copy link
Member

@bors r=Nilstrieb

@bors
Copy link
Contributor

bors commented May 5, 2023

📌 Commit f20738d has been approved by Nilstrieb

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 May 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2023
Rollup of 8 pull requests

Successful merges:

 - rust-lang#110946 (avoid duplicating TLS state between test std and realstd)
 - rust-lang#110954 (Reject borrows of projections in ConstProp.)
 - rust-lang#111052 (Fix problems with backtraces in two ui tests.)
 - rust-lang#111132 (cleanup nll generalizer)
 - rust-lang#111173 (Still more encoder cleanups)
 - rust-lang#111187 (bootstrap: add llvm-project/runtimes to the sources)
 - rust-lang#111213 (Fixup "since" dates for `array_tuple_conv` feature)
 - rust-lang#111223 (Use `free-args` consistently in bootstrap)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b2ee088 into rust-lang:master May 5, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 5, 2023
@nnethercote nnethercote deleted the fix-ice-test branch May 5, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants