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

Don't build a broken/untested profiler runtime on mingw targets #122613

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

Zalathar
Copy link
Contributor

Context: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Why.20build.20a.20broken.2Funtested.20profiler.20runtime.20on.20mingw.3F

#75872 added --enable-profiler to the x86_64-mingw job (to cause some additional tests to run), but had to also add //@ ignore-windows-gnu to all of the tests that rely on the profiler runtime actually working, because it's broken on that target.

We can achieve a similar outcome by going through all the //@ needs-profiler-support tests that don't actually need to produce/run a binary, and making them use -Zno-profiler-runtime instead, so that they can run even in configurations that don't have the profiler runtime available. Then we can remove --enable-profiler from x86_64-mingw, and still get the same amount of testing.

This PR also removes --enable-profiler from the mingw dist builds, since it is broken/untested on that target. Those builds have had that flag for a very long time.

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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 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-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. labels Mar 17, 2024
@Zalathar
Copy link
Contributor Author

@mati865 mentioned that this change might require some extra manual checking, to verify that the profiler runtime in the dist builds is sufficiently broken that we can happily remove it.

There's no rush, so it's fine for this PR to sit around for a while until it's convenient for that to happen.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@nnethercote
Copy link
Contributor

Looks ok to me though I am not familiar with any of these stuff. @mati865, if you could review as well that would be appreciated :)

@mati865
Copy link
Contributor

mati865 commented Mar 24, 2024

Linked final binary with binutils:

$ cargo rustc -- -C instrument-coverage
    Updating crates.io index
  Downloaded whoami v1.2.1
  Downloaded 1 crate (11.0 KB) in 0.47s
   Compiling whoami v1.2.1
   Compiling hello v0.1.0 (D:\tmp\hello)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.39s

$ target/debug/hello.exe
bash: target/debug/hello.exe: cannot execute binary file: Exec format error`

LLD yields "less bad" result but since the library shipped by Rust is still miscompiled by GCC/Binutils the resulting profraw file is malformed (the binary itself works fine):

$ RUSTFLAGS="-C link-arg=-fuse-ld=lld" cargo llvm-cov
info: cargo-llvm-cov currently setting cfg(coverage) and cfg(coverage_nightly); you can opt-out it by passing --no-cfg-coverage and --no-cfg-coverage-nightly
   Compiling whoami v1.2.1
   Compiling hello v0.1.0 (D:\tmp\hello)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.58s
     Running unittests src\main.rs (target\llvm-cov-target\debug\deps\hello-637c13bc18a191d0.exe)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

warning: D:\tmp\hello\target\llvm-cov-target\hello-18720-12256372789117787786_0.profraw: malformed instrumentation profile data: function name is empty
error: no profile can be merged
error: failed to merge profile data: process didn't exit successfully: `'C:\Users\mateusz\.rustup\toolchains\nightly-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\bin\llvm-profdata.exe' merge -sparse -f 'D:\tmp\hello\target\llvm-cov-target\hello-profraw-list' -o 'D:\tmp\hello\target\llvm-cov-target\hello.profdata'` (exit code: 1)
Some offtopic

Also noticed gnullvm targets might have regressed at some point (or this quickly created setup is not quite right which is more probable) but it doesn't matter for this PR:

$ cargo new --lib test_lib

$ cd test_lib/

$ PATH=~/.cargo/bin:/h/msys64/clang64/bin:$PATH cargo +stage1_gnullvm llvm-cov
info: cargo-llvm-cov currently setting cfg(coverage) and cfg(coverage_nightly); you can opt-out it by passing --no-cfg-coverage and --no-cfg-coverage-nightly
   Compiling test_lib v0.1.0 (D:\tmp\test_lib)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.32s
     Running unittests src\lib.rs (target\llvm-cov-target\debug\deps\test_lib-3d83b996113588e9.exe)

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                               0                 0         -           0                 0         -           0                 0         -           0                 0         -

@bors
Copy link
Contributor

bors commented Mar 25, 2024

☔ The latest upstream changes (presumably #123029) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link
Contributor

I will be away for 3 weeks, but if/when this reaches a point where @mati865 is happy with it then you have my r+.

@mati865
Copy link
Contributor

mati865 commented Mar 25, 2024

Sorry if it's not obvious enough but I have confirmed the profiler library shipped with Rust is hopelessly broken for this target and there is no point in shipping it.

@Zalathar
Copy link
Contributor Author

Zalathar commented May 3, 2024

Whoops, I had forgotten about this.

Rebased to account for #124175, which moved the job definitions out of ci.yml and into jobs.yml.

@Zalathar

This comment was marked as outdated.

@bors
Copy link
Contributor

bors commented May 4, 2024

☔ The latest upstream changes (presumably #124345) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jun 13, 2024

☔ The latest upstream changes (presumably #125165) made this pull request unmergeable. Please resolve the merge conflicts.

@Zalathar
Copy link
Contributor Author

@nnethercote According to @mati865 #122613 (comment) the shipped profiler is hopelessly broken on mingw, so we should be able to move forward with removing it.

@nnethercote
Copy link
Contributor

Repeating my comment from above:

if/when this reaches a point where @mati865 is happy with it then you have my r+.

:)

@Zalathar
Copy link
Contributor Author

Repeating my comment from above:

if/when this reaches a point where @mati865 is happy with it then you have my r+.

:)

I don't understand what you mean by this.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#122613 (Don't build a broken/untested profiler runtime on mingw targets)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)

Failed merges:

 - rust-lang#126388 (const-eval: make lint scope computation consistent)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r-
#126412 (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 Jun 13, 2024
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jun 14, 2024
@Zalathar
Copy link
Contributor Author

OK, I think I see what's happening.

Because I removed //@ needs-profiler-runtime, this test is now running on jobs that weren't running it before, which is expected.

On x86_64-gnu-nopt, the lines expected by filecheck appear in the wrong order, so the check fails. They appear to be in the wrong order on other jobs as well, but this is masked by the presence of multiple CGUs, allowing filecheck to find and match the lines in order from different CGUs.

It should be fine to just change CHECK: to CHECK-DAG: so that filecheck will accept the lines in either order, because the order isn't actually significant.

@rust-log-analyzer

This comment has been minimized.

For PGO/coverage tests that don't need to build or run an actual artifact, we
can use `-Zno-profiler-runtime` to run the test even when the profiler runtime
is not available.
The profiler runtime is no longer built in mingw test jobs, so these tests
should naturally be skipped by `//@ needs-profiler-support`.
@Zalathar
Copy link
Contributor Author

I've pushed a fix, but I'm waiting for #114917 to confirm that the fix actually works.

@Zalathar
Copy link
Contributor Author

The fixed version of tests/codegen/instrument-coverage/instrument-coverage.rs has been confirmed to work on x86_64-gnu-nopt.

Other than that and a rebase, no changes.

@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 Jun 14, 2024
@nnethercote
Copy link
Contributor

Fire when ready!

@bors delegate=Zalathar

@bors
Copy link
Contributor

bors commented Jun 14, 2024

✌️ @Zalathar, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

@Zalathar
Copy link
Contributor Author

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Jun 14, 2024

📌 Commit 186d94d has been approved by nnethercote

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 Jun 14, 2024
@bors
Copy link
Contributor

bors commented Jun 15, 2024

⌛ Testing commit 186d94d with merge 3fc81da...

@bors
Copy link
Contributor

bors commented Jun 15, 2024

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 3fc81da to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2024
@bors bors merged commit 3fc81da into rust-lang:master Jun 15, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 15, 2024
@Zalathar Zalathar deleted the profiler branch June 15, 2024 02:35
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3fc81da): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -3.0%)

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)
- - 0
Improvements ✅
(primary)
-3.0% [-3.0%, -3.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.0% [-3.0%, -3.0%] 1

Cycles

Results (secondary -3.0%)

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.6%, -2.4%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 671.997s -> 671.602s (-0.06%)
Artifact size: 320.43 MiB -> 320.43 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants