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

compiletest: "fix" FileCheck with --allow-unused-prefixes #84883

Merged
merged 2 commits into from
May 19, 2021

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented May 3, 2021

The default of --allow-unused-prefixes used to be false, but in LLVM
change 87dbdd2 (https://reviews.llvm.org/D95849) the default became
true. I'm not quite sure how we could do better here (specifically not
providing the CHECK prefix when it's not needed), but this seems to work
for now.

This reduces codegen test failures against LLVM HEAD from 31 cases to 5.

The default of --allow-unused-prefixes used to be false, but in LLVM
change 87dbdd2 (https://reviews.llvm.org/D95849) the default became
true. I'm not quite sure how we could do better here (specifically not
providing the CHECK prefix when it's not needed), but this seems to work
for now.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2021
@durin42
Copy link
Contributor Author

durin42 commented May 3, 2021

It wasn't clear to me how I should gate this flag on LLVM >= 13, but the flag was new in https://reviews.llvm.org/D90281, which is dated October 2020 so it's a recent flag. I assume we need to do some detection on that, but how? Can someone guide me on that?

Thanks!

@Mark-Simulacrum
Copy link
Member

r? @nikic as you had comments on the LLVM thread, and I'm not sure if we want to follow with an allow or perhaps fix our tests.

@nikic
Copy link
Contributor

nikic commented May 6, 2021

Can you maybe share an example of a test that currently fails without the flag?

@durin42
Copy link
Contributor Author

durin42 commented May 6, 2021

https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/1517#08add164-76ca-4964-ad3d-eee03fd98059 has around 25. An example from there:

---- [codegen] codegen/abi-efiapi.rs#arm stdout ----
  |  
  | error in revision `arm`: verification with 'FileCheck' failed
  | status: exit status: 2
  | command: "/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/build/x86_64-unknown-linux-gnu/test/codegen/abi-efiapi.arm/abi-efiapi.ll" "/var/lib/buildkite-agent/builds/rust-llvm-integrate/llvm-project/rust-llvm-integrate-prototype/src/test/codegen/abi-efiapi.rs" "--check-prefixes" "CHECK,arm"
  | stdout:
  | ------------------------------------------
  |  
  | ------------------------------------------
  | stderr:
  | ------------------------------------------
  | error: no check strings found with prefix 'CHECK:'
  |  
  | ------------------------------------------ 

I think the problem is the unconditional inclusion of CHECK in the --check-prefixes flag, but I'm not sure how to determine when it should be included.

@nikic
Copy link
Contributor

nikic commented May 7, 2021

Thanks for the example. So the problem is that for tests with revisions, we translate the revisions to FileCheck prefixes, while also retaining CHECK as a common prefix. And we do have tests that make use of both CHECK and revision prefixes at the same time.

So I think passing --allow-unused-prefixes is the right thing to do for our usage. You should be able to gate on LLVM by checking config.llvm_version, which will be something like Some(130000) for LLVM 13.

@durin42
Copy link
Contributor Author

durin42 commented May 17, 2021

Updated - note that as of today rustc HEAD won't compile against LLVM HEAD, but you can test this change if you want against LLVM commit d2a291a - I'll try and work on the build-fix later today so we can see the remaining codegen bugs against head easily.

@durin42
Copy link
Contributor Author

durin42 commented May 17, 2021

Alternatively, you can wait for #85416 to be in and then this should be easy to test against LLVM/Rust HEADs.

@nikic
Copy link
Contributor

nikic commented May 18, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit 3035816 has been approved by nikic

@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 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 18, 2021
compiletest: "fix" FileCheck with --allow-unused-prefixes

The default of --allow-unused-prefixes used to be false, but in LLVM
change 87dbdd2 (https://reviews.llvm.org/D95849) the default became
true. I'm not quite sure how we could do better here (specifically not
providing the CHECK prefix when it's not needed), but this seems to work
for now.

This reduces codegen test failures against LLVM HEAD from 31 cases to 5.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 19, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#83366 (Stabilize extended_key_value_attributes)
 - rust-lang#83767 (Fix v0 symbol mangling bug)
 - rust-lang#84883 (compiletest: "fix" FileCheck with --allow-unused-prefixes)
 - rust-lang#85274 (Only pass --[no-]gc-sections if linker is GNU ld.)
 - rust-lang#85297 (bootstrap: build cargo only if requested in tools)
 - rust-lang#85396 (rustdoc: restore header sizes)
 - rust-lang#85425 (Fix must_use on `Option::is_none`)
 - rust-lang#85438 (Fix escape handling)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 37ecede into rust-lang:master May 19, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 19, 2021
@durin42 durin42 deleted the allow-unused-prefixes branch August 24, 2021 14:07
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants