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.

Sign up for GitHub

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

Jump to bottom

Port backtrace dylib-dep test to a ui test #122958

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Port backtrace dylib-dep test to a ui test #122958

merged 1 commit into from
Mar 27, 2024

Conversation

jieyouxu
Copy link
Member

Original test: dylib-dep
Part of #122899.

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 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. A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 23, 2024
@jieyouxu jieyouxu marked this pull request as draft March 23, 2024 18:13
@jieyouxu jieyouxu removed the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Mar 23, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Mar 23, 2024
workingjubilee
workingjubilee reviewed Mar 23, 2024
View reviewed changes
tests/ui/debuginfo/backtrace-dylib-dep.rs Outdated
@@ -7,6 +7,7 @@
//@ compile-flags: -g -Copt-level=0 -Cstrip=none
//@ aux-crate: dylib_dep_helper=dylib-dep-helper.rs
//@ aux-crate: auxiliary=dylib-dep-helper-aux.rs
//@ ignore-wasm32 spawning processes is not supported
Copy link
Member

Choose a reason for hiding this comment

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

is there no "needs the ability to spawn processes" directive for compiletest currently, @jieyouxu? wasm32 is not the only platform that has a limited ability to do this.

Copy link
Member Author

@jieyouxu jieyouxu Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually that isn't quite right -- I'm not spawning processes like in tests/ui/backtrace.rs, so the previous failure on wasm was for a different reason. At least, ignore-wasm32 shouldn't be because of no process spawning support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I just need to set WASMTIME_BACKTRACE_DETAILS=1 here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the wasmtime test runner doesn't have debug-info enabled, I think?

@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from 4eea450 to 11fc9d5 Compare March 23, 2024 19:38
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 23, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu

This comment was marked as outdated.

Sign in to view
@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch 2 times, most recently from 021eb7f to 343448e Compare March 24, 2024 16:17
@jieyouxu jieyouxu marked this pull request as ready for review March 24, 2024 16:18
@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from 343448e to 8dc10da Compare March 24, 2024 16:42
@petrochenkov
Copy link
Contributor

r? @workingjubilee

@workingjubilee
Copy link
Member

Oh wow this is quite thorough! And also that suite of //@ ignores makes me think this is going to get rejected so hard by various platforms!

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 25, 2024

📌 Commit 8dc10da has been approved by workingjubilee

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 Mar 25, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 25, 2024
@workingjubilee
Rollup merge of rust-lang#122958 - jieyouxu:port-backtrace-dylib-dep,…
0aa8ed8 
… r=workingjubilee

Port backtrace dylib-dep test to a ui test

Original test: [dylib-dep](https://github.com/rust-lang/backtrace-rs/tree/6fa4b85b9962c3e1be8c2e5cc605cd078134152b/crates/dylib-dep)
Part of rust-lang#122899.
@workingjubilee workingjubilee mentioned this pull request Mar 25, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
@bors
Auto merge of rust-lang#123054 - workingjubilee:rollup-hi8x0ut, r=wor…
1a69751 
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#122707 (Fix a typo in the alloc::string::String docs)
 - rust-lang#122769 (extend comments for reachability set computation)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122950 (Add regression tests for rust-lang#101903)
 - rust-lang#122958 (Port backtrace dylib-dep test to a ui test)
 - rust-lang#123039 (Update books)
 - rust-lang#123044 (`Instance` is `Copy`)
 - rust-lang#123051 (did I mention that tests are super cool? )

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu jieyouxu marked this pull request as draft March 25, 2024 21:52
@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from 8dc10da to e0f17c0 Compare March 25, 2024 21:58
@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 25, 2024 •
edited
Loading

And I got crosseyed between this and the other test which has //@ ignore-windows original test is ignore-windows.

Heh, the other line-tables-only test specifically wanted to test line number tables with clang -g1 (which AFAIK the flag also works with gcc to produce line number tables). I got confused between this and the other tests as well apparently, this test doesn't even need the -g1 because it doesn't rely on the C helper!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from 8153c39 to d6a0c2a Compare March 25, 2024 23:03
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

@jieyouxu try -Zforce-frame-pointers

@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from d6a0c2a to 8101502 Compare March 26, 2024 15:17
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from 8101502 to eaa6052 Compare March 26, 2024 15:38
@jieyouxu
Copy link
Member Author

Thank you msvc

@jieyouxu jieyouxu force-pushed the port-backtrace-dylib-dep branch from eaa6052 to 7764e8a Compare March 26, 2024 19:07
@jieyouxu
Copy link
Member Author

-Cforce-frame-pointers=yes seems to have worked!
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2024
@workingjubilee
Copy link
Member

Holy shit! Let's give it a go.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 26, 2024

📌 Commit 7764e8a has been approved by workingjubilee

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 Mar 26, 2024
@workingjubilee workingjubilee marked this pull request as ready for review March 26, 2024 21:45
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 26, 2024
@workingjubilee
Rollup merge of rust-lang#122958 - jieyouxu:port-backtrace-dylib-dep,…
4cd34b1 
… r=workingjubilee

Port backtrace dylib-dep test to a ui test

Original test: [dylib-dep](https://github.com/rust-lang/backtrace-rs/tree/6fa4b85b9962c3e1be8c2e5cc605cd078134152b/crates/dylib-dep)
Part of rust-lang#122899.
@workingjubilee workingjubilee mentioned this pull request Mar 26, 2024
Closed
@bors
Copy link
Contributor

bors commented Mar 27, 2024

⌛ Testing commit 7764e8a with merge 435b525...

@bors
Copy link
Contributor

bors commented Mar 27, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 435b525 to master...

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

Finished benchmarking commit (435b525): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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)		 - - 0
Improvements ✅
(primary)		 -0.6% [-0.6%, -0.6%] 4
Improvements ✅
(secondary)		 -0.4% [-0.5%, -0.2%] 10
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 4

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)		 3.3% [0.5%, 4.9%] 6
Regressions ❌
(secondary)		 2.9% [1.8%, 4.8%] 31
Improvements ✅
(primary)		 -0.8% [-0.8%, -0.8%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.7% [-0.8%, 4.9%] 7

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)		 1.0% [0.5%, 1.6%] 6
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.0% [0.5%, 1.6%] 6

Binary size

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

Bootstrap: 670.032s -> 670.407s (0.06%)
Artifact size: 315.61 MiB -> 315.71 MiB (0.03%)

@jieyouxu jieyouxu deleted the port-backtrace-dylib-dep branch March 27, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

Assignees

@workingjubilee workingjubilee

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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jieyouxu @rustbot @rust-log-analyzer @petrochenkov @workingjubilee @bors @rust-timer