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

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

Merged
merged 1 commit into from
Nov 15, 2024
+15 −3
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions tests/run-make/pointer-auth-link-with-c-lto-clang/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@
//@ ignore-cross-compile
// Reason: the compiled binary is executed

use run_make_support::{clang, env_var, llvm_ar, run, rustc, static_lib_name};
use run_make_support::{clang, env_var, llvm_ar, llvm_objdump, run, rustc, static_lib_name};

static PAUTH_A_KEY_PATTERN: &'static str = "paciasp";
static PAUTH_B_KEY_PATTERN: &'static str = "pacibsp";

fn main() {
clang()
.arg("-v")
.lto("thin")
.arg("-mbranch-protection=bti+pac-ret+leaf")
.arg("-O2")
.arg("-mbranch-protection=bti+pac-ret+b-key+leaf")
.arg("-c")
.out_exe("test.o")
.input("test.c")
Expand All @@ -32,5 +34,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 •
edited
Loading

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.

llvm_objdump()
.disassemble()
.input("test.bin")
.run()
.assert_stdout_contains_regex(PAUTH_A_KEY_PATTERN)
.assert_stdout_contains_regex(PAUTH_B_KEY_PATTERN);

// Check that the binary actually runs
run("test.bin");
}
Loading