Skip to content

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jul 3, 2023

This change makes debuginfo tests more user friendly. Changes:

  • Print all lines that fail to match the patterns instead of just the first
  • Provide better error messages that also say what did match
  • Strip leading whitespace from directives so they are not skipped if indented
  • Improve documentation and improve nesting on some related items

As an example, given the following intentional fail (and a few not shown):

// from tests/debuginfo/rc_arc.rs

// cdb-command:dx rc,d
// cdb-check:rc,d             : 111 [Type: alloc::rc::Rc<i32>]
// cdb-check:    [Reference count] : 11 [Type: core::cell FAIL::Cell<usize>]
// cdb-check:    [Weak reference count] : 2 [Type: core::cell FAIL::Cell<usize>]

The current output (tested in #113313) will show:

2023-07-04T08:10:00.1939267Z ---- [debuginfo-cdb] tests\debuginfo\rc_arc.rs stdout ----
2023-07-04T08:10:00.1942182Z
2023-07-04T08:10:00.1957463Z error: line not found in debugger output:     [Reference count] : 11 [Type: core:: cell FAIL::Cell<usize>]
2023-07-04T08:10:00.1958272Z status: exit code: 0

With this change, you are able to see all failures in that check group, as well as what parts were successful. The output is now:

2023-07-04T09:45:57.2514224Z error: check directive(s) from `C:\a\rust\rust\tests\debuginfo\rc_arc.rs` not found in debugger output. errors:
2023-07-04T09:45:57.2514631Z     (rc_arc.rs:31) `    [Reference count] : 11 [Type: core::cell FAIL::Cell<usize>]`
2023-07-04T09:45:57.2514908Z     (rc_arc.rs:32) `    [Weak reference count] : 2 [Type: core::cell FAIL::Cell<usize>]`
2023-07-04T09:45:57.2515181Z     (rc_arc.rs:41) `    [Reference count] : 21 [Type: core::sync::atomic FAIL::AtomicUsize]`
2023-07-04T09:45:57.2515452Z     (rc_arc.rs:50) `dyn_rc,d         [Type: alloc::rc::Rc<dyn$<core::fmt FAIL::Debug> >]`
2023-07-04T09:45:57.2515695Z the following subset of check directive(s) was found successfully::
2023-07-04T09:45:57.2516080Z     (rc_arc.rs:30) `rc,d             : 111 [Type: alloc::rc::Rc<i32>]`
2023-07-04T09:45:57.2516312Z     (rc_arc.rs:35) `weak_rc,d        : 111 [Type: alloc::rc::Weak<i32>]`
2023-07-04T09:45:57.2516555Z     (rc_arc.rs:36) `    [Reference count] : 11 [Type: core::cell::Cell<usize>]`
2023-07-04T09:45:57.2516881Z     (rc_arc.rs:37) `    [Weak reference count] : 2 [Type: core::cell::Cell<usize>]`
...

Which makes it easier to see what did and didn't succeed without manual comparison against the source test file.

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 3, 2023
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 4, 2023

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 4, 2023
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the debuginfo-better-output branch from 3b612a4 to 35defa2 Compare July 4, 2023 01:46
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the debuginfo-better-output branch 2 times, most recently from 796dafb to a4698c2 Compare July 4, 2023 03:49
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the debuginfo-better-output branch 2 times, most recently from 533b46b to 7871ac3 Compare July 4, 2023 07:18
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the debuginfo-better-output branch 2 times, most recently from 2726349 to c485ca4 Compare July 4, 2023 08:59
@tgross35 tgross35 changed the title Experiment making debugtests print all mismatched lines Update debuginfo test runner to provide more useful output Jul 4, 2023
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the debuginfo-better-output branch from c485ca4 to 4e1dc23 Compare July 4, 2023 10:03
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 4, 2023

Alright, the output looks good so I have reverted the intentional errors and CI changes. Ready for a look.

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 4, 2023
@tgross35 tgross35 marked this pull request as ready for review July 4, 2023 10:04
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2023
@tgross35 tgross35 force-pushed the debuginfo-better-output branch from 4e1dc23 to e62dcf6 Compare July 4, 2023 10:11
This change makes debuginfo tests more user friendly. Changes:

-   Print all lines that fail to match the patterns instead of just
    the first
-   Provide better error messages that also say what did match
-   Strip leading whitespace from directives so they are not skipped if
    indented
-   Improve documentation and improve nesting on some related items

As an example, given the following debuginfo test with intentional
fails:

```rust
// from tests/debuginfo/rc_arc.rs

// cdb-command:dx rc,d
// cdb-check:rc,d             : 111 [Type: alloc::rc::Rc<i32>]
// cdb-check:    [Reference count] : 11 [Type: core::cell FAIL::Cell<usize>]
// cdb-check:    [Weak reference count] : 2 [Type: core::cell FAIL::Cell<usize>]

// ...
```

The current output (tested in rust-lang#113313) only shows the first mismatch:

```
2023-07-04T08:10:00.1939267Z ---- [debuginfo-cdb] tests\debuginfo\rc_arc.rs stdout ----
2023-07-04T08:10:00.1942182Z
2023-07-04T08:10:00.1957463Z error: line not found in debugger output:     [Reference count] : 11 [Type: core::cell FAIL::Cell<usize>]
2023-07-04T08:10:00.1958272Z status: exit code: 0
```

With this change, you are able to see all failures in that check
group, as well as what parts were successful. The output is now:

```
2023-07-04T09:45:57.2514224Z error: check directive(s) from `C:\a\rust\rust\tests\debuginfo\rc_arc.rs` not found in debugger output. errors:
2023-07-04T09:45:57.2514631Z     (rc_arc.rs:31) `    [Reference count] : 11 [Type: core::cell FAIL::Cell<usize>]`
2023-07-04T09:45:57.2514908Z     (rc_arc.rs:32) `    [Weak reference count] : 2 [Type: core::cell FAIL::Cell<usize>]`
2023-07-04T09:45:57.2515181Z     (rc_arc.rs:41) `    [Reference count] : 21 [Type: core::sync::atomic FAIL::AtomicUsize]`
2023-07-04T09:45:57.2515452Z     (rc_arc.rs:50) `dyn_rc,d         [Type: alloc::rc::Rc<dyn$<core::fmt FAIL::Debug> >]`
2023-07-04T09:45:57.2515695Z the following subset of check directive(s) was found successfully::
2023-07-04T09:45:57.2516080Z     (rc_arc.rs:30) `rc,d             : 111 [Type: alloc::rc::Rc<i32>]`
2023-07-04T09:45:57.2516312Z     (rc_arc.rs:35) `weak_rc,d        : 111 [Type: alloc::rc::Weak<i32>]`
2023-07-04T09:45:57.2516555Z     (rc_arc.rs:36) `    [Reference count] : 11 [Type: core::cell::Cell<usize>]`
2023-07-04T09:45:57.2516881Z     (rc_arc.rs:37) `    [Weak reference count] : 2 [Type: core::cell::Cell<usize>]`
...
```

Which makes it easier to see what did and didn't succeed without
manual comparison against the source test file.
@tgross35 tgross35 force-pushed the debuginfo-better-output branch from e62dcf6 to b0a18cb Compare July 4, 2023 10:12
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Jul 8, 2023

📌 Commit b0a18cb 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2023
@bors
Copy link
Collaborator

bors commented Jul 9, 2023

⌛ Testing commit b0a18cb with merge ba37a69...

@bors
Copy link
Collaborator

bors commented Jul 9, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing ba37a69 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2023
@bors bors merged commit ba37a69 into rust-lang:master Jul 9, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 9, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ba37a69): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.6% [5.6%, 5.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [1.2%, 2.2%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [-2.6%, 2.2%] 9

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.6%, -2.0%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 658.609s -> 658.544s (-0.01%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 9, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 9, 2023

I don’t think this PR can have anything to do with the regression since it’s only a test change

@tgross35 tgross35 deleted the debuginfo-better-output branch July 9, 2023 16:24
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. perf-regression Performance regression. 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants