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

tests: Test pac-ret flag merging on clang with LTO #133045

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

mrkajetanp
Copy link
Contributor

@mrkajetanp mrkajetanp commented Nov 14, 2024

Extend the test for pac-ret with clang and LTO by checking that different branch protection flags are preserved after the LTO step. There was an issue in older LLVM versions that was causing this to behave incorrectly.

try-job: aarch64-gnu-debug

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
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-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@jieyouxu
Copy link
Member

@bors try

@@ -32,5 +35,15 @@ fn main() {
.input("test.rs")
.output("test.bin")
.run();

// Check that both a-key and b-key pac-ret survived LTO
Copy link
Member

Choose a reason for hiding this comment

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

Question: is there any background / links for this? I'm fine with the change, but it's even better if there's any kind of context to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, this is the context:
llvm/llvm-project@1782810

TLDR: there was a bug in LLVM where during the LTO step instructions like "pacibsp" would get replaced with "paciasp" due to LLVM module flag merging behaviour. As currently written the assembly output should have no PAuth on library functions, paciasp at the start of main and pacibsp at the start of foo.

It is worrying that the test is failing, I'll try to reproduce it locally.

Copy link
Member

@jieyouxu jieyouxu Nov 15, 2024

Choose a reason for hiding this comment

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

Thanks for the context. And I agree, the test failing is... suspicious lol. Can you please include your reply as a test comment to help a $FUTURE_READER not have to find this PR to understand the context?

Copy link
Member

Choose a reason for hiding this comment

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

Also it might help if you do something like let clang_version = clang().arg("-v").run(); and then print that to double-check what clang version CI has.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay this is very funny

00000000000677a4 <test::main>:
   677a4: d503233f      paciasp
   677a8: d50323bf      autiasp
   677ac: d65f03c0      ret

The test fails because the compiler optimises away the entire call to foo and its pac instructions along with it. I'll fix it..

Copy link
Member

Choose a reason for hiding this comment

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

Right... That can explain it lol. black_box to the rescue.

@bors
Copy link
Contributor

bors commented Nov 15, 2024

⌛ Trying commit ab13be7 with merge bc93078...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
…-merge, r=<try>

tests: Test pac-ret flag merging on clang with LTO

Extend the test for pac-ret with clang and LTO by checking that different branch protection flags are preserved after the LTO step. There was an issue in older LLVM versions that was causing this to behave incorrectly.

try-job: aarch64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 15, 2024

💔 Test failed - checks-actions

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2024
Extend the test for pac-ret with clang and LTO by checking that
different branch protection flags are preserved after the LTO step.
There was an issue in older LLVM versions that was causing this to
behave incorrectly.

Tests the LLVM behaviour added in:
llvm/llvm-project@1782810
@mrkajetanp mrkajetanp force-pushed the pauth-test-clang-lto-flag-merge branch from ab13be7 to 194471c Compare November 15, 2024 14:47
@mrkajetanp
Copy link
Contributor Author

Dropping that opt flag makes it work for me, could you have bors try it again?

@jieyouxu
Copy link
Member

Sure. @bors try

@bors
Copy link
Contributor

bors commented Nov 15, 2024

⌛ Trying commit 194471c with merge 9002bfd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
…-merge, r=<try>

tests: Test pac-ret flag merging on clang with LTO

Extend the test for pac-ret with clang and LTO by checking that different branch protection flags are preserved after the LTO step. There was an issue in older LLVM versions that was causing this to behave incorrectly.

try-job: aarch64-gnu-debug
@bors
Copy link
Contributor

bors commented Nov 15, 2024

☀️ Try build successful - checks-actions
Build commit: 9002bfd (9002bfd553268869bd638084c35ca9946b301b7f)

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

So it turns out not optimizing out the function that we want to check makes it easier to test for assembly inside said function 😆. Thanks!

@jieyouxu
Copy link
Member

@bors r+ rollup (only runs in one CI job that was tested here).

@bors
Copy link
Contributor

bors commented Nov 15, 2024

📌 Commit 194471c has been approved by jieyouxu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#132817 (Recurse into APITs in `impl_trait_overcaptures`)
 - rust-lang#133021 (Refactor `configure_annotatable`)
 - rust-lang#133045 (tests: Test pac-ret flag merging on clang with LTO)
 - rust-lang#133049 (Change Visitor::visit_precise_capturing_arg so it returns a Visitor::Result)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 249a910 into rust-lang:master Nov 15, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
Rollup merge of rust-lang#133045 - mrkajetanp:pauth-test-clang-lto-flag-merge, r=jieyouxu

tests: Test pac-ret flag merging on clang with LTO

Extend the test for pac-ret with clang and LTO by checking that different branch protection flags are preserved after the LTO step. There was an issue in older LLVM versions that was causing this to behave incorrectly.

try-job: aarch64-gnu-debug
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 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.

5 participants