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

test(pgo): ensure PGO works #14859

Merged
merged 1 commit into from
Nov 26, 2024
Merged

test(pgo): ensure PGO works #14859

merged 1 commit into from
Nov 26, 2024

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

This is a regression test to prevent issues like #7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit different PGO function missing warnings.

How should we test and review this PR?

Not sure how brittle it is. We can optionally run it only on Cargo's CI?

cc #14830

@rustbot
Copy link
Collaborator

rustbot commented Nov 26, 2024

r? @ehuss

rustbot has assigned @ehuss.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2024
@weihanglo
Copy link
Member Author

weihanglo commented Nov 26, 2024

This is how LLVM warnings look like if we hash RUSTFLAGS into -Cmetadata

---- expected: tests/testsuite/pgo.rs:98:27
++++ actual:   stderr
   1    1 | [COMPILING] foo v0.0.0 ([ROOT]/foo)
   2    2 | [RUNNING] `rustc [..]-Cprofile-use=[ROOT]/foo/target/merged.profdata -Cllvm-args=-pgo-warn-missing-function`
        3 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN3std2rt10lang_start17h326bc87852528165E Hash = 742261418966908927 up to 0 count discarded
        4 + 
        5 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN3std2rt10lang_start28_$u7b$$u7b$closure$u7d$$u7d$17hf49388a32a52a388E Hash = 7422[614](https://github.com/weihanglo/cargo/actions/runs/12032501015/job/33544505332#step:11:615)18966908927 up to 0 count discarded
        6 + 
        7 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN3std3sys9backtrace28__rust_begin_short_backtrace17hfbc657bfed1d0a0fE Hash = 170957022131388415 up to 0 count discarded
        8 + 
        9 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN4core3ops8function6FnOnce40call_once$u7b$$u7b$vtable.shim$u7d$$u7d$17h392723b7040dbd77E Hash = 742261418966908927 up to 0 count discarded
       10 + 
       11 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN3foo9fibonacci17he8ee274359179b8dE Hash = 1063705160746808702 up to 0 count discarded
       12 + 
       13 + [WARNING] foo.6d8fa1cc59b84deb-cgu.0: no profile data available for function _ZN3foo4main17hc72aa038f08b88faE Hash = 1124680652719534771 up to 0 count discarded
       14 + 
       15 + [WARNING] `foo` (bin "foo") generated 6 warnings
   3   16 | [FINISHED] `release` profile [optimized] target(s) in [ELAPSED]s

This is a regression test to prevent issues like rust-lang#7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit different PGO function missing warnings.
@epage epage added this pull request to the merge queue Nov 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024
@epage epage added this pull request to the merge queue Nov 26, 2024
Merged via the queue into rust-lang:master with commit 917e646 Nov 26, 2024
20 checks passed
}

let Some(llvm_profdata) = llvm_profdata() else {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to enforce that this exists in CI? Otherwise we don't know if this test is ever actually running.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I've considered that and found it needs more work than the current one.

I'll try to improve it!

Comment on lines +25 to +31
#[cargo_test]
fn pgo_works() {
if cfg!(not(target_os = "linux")) {
// macOS may emit different LLVM PGO warnings.
// Windows LLVM has different requirements.
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure to use the ignore attribute when disabling a test? Otherwise it can be confusing when tests don't seem to change when you expect them to, are say that they are passing when you expect them to fail.

I realize there's still a big hole when llvm-profdata is missing. We could maybe add that to our requires mechanism?

Suggested change
#[cargo_test]
fn pgo_works() {
if cfg!(not(target_os = "linux")) {
// macOS may emit different LLVM PGO warnings.
// Windows LLVM has different requirements.
return;
}
#[cargo_test]
// macOS may emit different LLVM PGO warnings.
// Windows LLVM has different requirements.
#[cfg_attr(not(target_os = "linux"), ignore = "linux only")]
fn pgo_works() {

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 30, 2024
Update cargo

8 commits in 4c39aaff66862cc0da52fe529aa1990bb8bb9a22..3908f64086a3d7b9af8d87b4da2bd100776c3e61
2024-11-25 16:36:17 +0000 to 2024-11-29 17:32:44 +0000
- chore(deps): update msrv (rust-lang/cargo#14867)
- fix(fix): Migrate cargo script manifests across editions (rust-lang/cargo#14864)
- feat(toml): Allow adding/removing from cargo scripts (rust-lang/cargo#14857)
- Add future-incompat warning against keywords in cfgs and add raw-idents (rust-lang/cargo#14671)
- test(build-std): download deps first (rust-lang/cargo#14861)
- test(pgo): ensure PGO works (rust-lang/cargo#14859)
- git-fetch-with-cli: Set `GIT_DIR` for bare repository compatibility (rust-lang/cargo#14860)
- fix(build-std): always link to std when testing proc-macros (rust-lang/cargo#14850)
@weihanglo weihanglo deleted the pgo branch November 30, 2024 01:44
@rustbot rustbot added this to the 1.85.0 milestone Nov 30, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 30, 2024
### What does this PR try to resolve?

The new syntax is key-value pair like `requires = "rustfmt"`,
comparing to the previous `requires_<cmd>`.

This enables `cargo_test` to require that the CLI contains
non-alphabetic names, such as `llvm-profdata`.

### How should we test and review this PR?

CI passes

### Additional information

This is a request from
<#14859 (comment)>,
but I found it a bit tricky so split off this change first.
github-merge-queue bot pushed a commit that referenced this pull request Nov 30, 2024
### What does this PR try to resolve?

Address
#14859 (comment)

I found that detecting llvm-profdata [requires more
works](weihanglo#73), so do this first.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
Update cargo

17 commits in 4c39aaff66862cc0da52fe529aa1990bb8bb9a22..05f54fdc34310f458033af8a63ce1d699fae8bf6
2024-11-25 16:36:17 +0000 to 2024-12-03 03:14:12 +0000
- test(pgo): only run on nightly (rust-lang/cargo#14887)
- chore: Bump to 0.86.0; update changelog (rust-lang/cargo#14885)
- docs(ref): Finish a sentence on rust-versions (rust-lang/cargo#14884)
- chore(deps): update rust crate cargo_metadata to 0.19.0 (rust-lang/cargo#14878)
- chore(deps): update rust crate gix to 0.68.0 (rust-lang/cargo#14879)
- fix: Remove default registry reference in `info` cmd docs (rust-lang/cargo#14880)
- test(pgo): determine test runnability at compile time (rust-lang/cargo#14874)
- test: `requires` attribute accepts string literals for cmds (rust-lang/cargo#14875)
- chore(deps): update msrv (3 versions) to v1.81 (rust-lang/cargo#14871)
- chore(deps): update msrv (rust-lang/cargo#14867)
- fix(fix): Migrate cargo script manifests across editions (rust-lang/cargo#14864)
- feat(toml): Allow adding/removing from cargo scripts (rust-lang/cargo#14857)
- Add future-incompat warning against keywords in cfgs and add raw-idents (rust-lang/cargo#14671)
- test(build-std): download deps first (rust-lang/cargo#14861)
- test(pgo): ensure PGO works (rust-lang/cargo#14859)
- git-fetch-with-cli: Set `GIT_DIR` for bare repository compatibility (rust-lang/cargo#14860)
- fix(build-std): always link to std when testing proc-macros (rust-lang/cargo#14850)
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 6, 2024
Update cargo

17 commits in 4c39aaff66862cc0da52fe529aa1990bb8bb9a22..05f54fdc34310f458033af8a63ce1d699fae8bf6
2024-11-25 16:36:17 +0000 to 2024-12-03 03:14:12 +0000
- test(pgo): only run on nightly (rust-lang/cargo#14887)
- chore: Bump to 0.86.0; update changelog (rust-lang/cargo#14885)
- docs(ref): Finish a sentence on rust-versions (rust-lang/cargo#14884)
- chore(deps): update rust crate cargo_metadata to 0.19.0 (rust-lang/cargo#14878)
- chore(deps): update rust crate gix to 0.68.0 (rust-lang/cargo#14879)
- fix: Remove default registry reference in `info` cmd docs (rust-lang/cargo#14880)
- test(pgo): determine test runnability at compile time (rust-lang/cargo#14874)
- test: `requires` attribute accepts string literals for cmds (rust-lang/cargo#14875)
- chore(deps): update msrv (3 versions) to v1.81 (rust-lang/cargo#14871)
- chore(deps): update msrv (rust-lang/cargo#14867)
- fix(fix): Migrate cargo script manifests across editions (rust-lang/cargo#14864)
- feat(toml): Allow adding/removing from cargo scripts (rust-lang/cargo#14857)
- Add future-incompat warning against keywords in cfgs and add raw-idents (rust-lang/cargo#14671)
- test(build-std): download deps first (rust-lang/cargo#14861)
- test(pgo): ensure PGO works (rust-lang/cargo#14859)
- git-fetch-with-cli: Set `GIT_DIR` for bare repository compatibility (rust-lang/cargo#14860)
- fix(build-std): always link to std when testing proc-macros (rust-lang/cargo#14850)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants