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

[BOLT] Instrumentation clobbers used stack slot #61114

Closed
nikic opened this issue Mar 2, 2023 · 5 comments
Closed

[BOLT] Instrumentation clobbers used stack slot #61114

nikic opened this issue Mar 2, 2023 · 5 comments
Assignees
Labels

Comments

@nikic
Copy link
Contributor

nikic commented Mar 2, 2023

When instrumenting a PGO-optimized libLLVM.so, the instrumented binary binary crashes in _ZNK4llvm4Loop18isLoopSimplifyFormEv. See https://gist.github.com/nikic/e695bd62d1dd40e506f6365b935d417c for the instrumented and uninstrumented assembly of the function.

The problem is that the value spilled by mov %r8,-0x10(%rsp) is later clobbered by the pushes in the instrumentation sequence:

   0x00000000075873d9 <+975>:	push   %rax
   0x00000000075873da <+976>:	mov    $0x0,%eax
   0x00000000075873df <+981>:	lahf   
   0x00000000075873e0 <+982>:	push   %rax
   0x00000000075873e1 <+983>:	mov    $0x0,%eax
   0x00000000075873e6 <+988>:	seto   %al
   0x00000000075873e9 <+991>:	lock incq 0x57fa3d7(%rip)        # 0xcd817c8
   0x00000000075873f1 <+999>:	add    $0x7f,%al
   0x00000000075873f4 <+1002>:	pop    %rax
   0x00000000075873f5 <+1003>:	sahf   
   0x00000000075873f6 <+1004>:	pop    %rax

I've uploaded the libLLVM.so in question here: https://drive.google.com/file/d/1lAbqukTx7b1aPasrR7rP-p2nsoRQSgUa/view?usp=sharing

I've also included an opt binary and a test case which make it possible to test the instrumented/uninstrumented libraries as follows:

# Runs fine
LD_LIBRARY_PATH=$PWD valgrind ./opt -S -passes=simple-loop-unswitch < test.ll

# Instrument
mv libLLVM-16-rust-1.69.0-nightly.so libLLVM-16-rust-1.69.0-nightly.so.orig
llvm-bolt -instrument libLLVM-16-rust-1.69.0-nightly.so.orig -o libLLVM-16-rust-1.69.0-nightly.so --instrumentation-file-append-pid

# Produces "Conditional jump or move depends on uninitialised value(s)"
LD_LIBRARY_PATH=$PWD valgrind ./opt -S -passes=simple-loop-unswitch < test.ll
@nikic nikic added the BOLT label Mar 2, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 2, 2023

@llvm/issue-subscribers-bolt

@aaupov
Copy link
Contributor

aaupov commented Mar 2, 2023

Reproduced with

$ llvm-bolt -instrument libLLVM-16-rust-1.69.0-nightly.so.orig -o libLLVM-16-rust-1.69.0-nightly.so -funcs=_ZNK4llvm4Loop18isLoopSimplifyFormEv
$ LD_LIBRARY_PATH=$PWD valgrind ./opt -S -passes=simple-loop-unswitch < test.ll
...
==13120== Conditional jump or move depends on uninitialised value(s)          
==13120==    at 0xA400109: ??? (in /home/aaupov/llvm-build-debug/bolt_repro/libLLVM-16-rust-1.69.0-nightly.so)                                               
==13120==    by 0x1FFEFFD3B7: ???                                                                                                                            
...

@aaupov
Copy link
Contributor

aaupov commented Mar 2, 2023

BOLT recognizes the function as non-leaf. We assume we can clobber red zone if the function is non-leaf. But it looks like in this case the function has no calls in its body except a tail call at the end, so it can clobber its red zone. https://reviews.llvm.org/D145202 fixes this issue and I think is an appropriate solution.

@nikic
Copy link
Contributor Author

nikic commented Mar 3, 2023

Thanks! I've verified that this patch fixes the original build (I applied it on top of release/16.x).

@aaupov
Copy link
Contributor

aaupov commented Mar 3, 2023

Thank you for a reproducer and a test case!

@aaupov aaupov closed this as completed in 1e1dfbb Mar 3, 2023
llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Mar 3, 2023
…ls only

Allow a function with tail calls only to clobber its red zone.

Fixes llvm/llvm-project#61114.

Reviewed By: #bolt, yota9

Differential Revision: https://reviews.llvm.org/D145202

(cherry picked from commit 1e1dfbb)
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Mar 6, 2023
…ls only

Allow a function with tail calls only to clobber its red zone.

Fixes llvm/llvm-project#61114.

Reviewed By: #bolt, yota9

Differential Revision: https://reviews.llvm.org/D145202

(cherry picked from commit 1e1dfbb)
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 18, 2023
Upgrade to LLVM 16

This updates Rust to LLVM 16. It also updates our host compiler for dist-x86_64-linux to LLVM 16. The reason for that is that Bolt from LLVM 15 is not capable of compiling LLVM 16 (llvm/llvm-project#61114).

LLVM 16.0.0 has been [released](https://discourse.llvm.org/t/llvm-16-0-0-release/69326) on March 18, while Rust 1.70 will become stable on June 1.

Tested images: `dist-x86_64-linux`, `dist-riscv64-linux` (alt), `dist-x86_64-illumos`, `dist-various-1`, `dist-various-2`, `dist-powerpc-linux`, `wasm32`, `armhf-gnu`
Tested images until the usual IPv6 failures: `test-various`
saethlin pushed a commit to saethlin/miri that referenced this issue Mar 19, 2023
Upgrade to LLVM 16

This updates Rust to LLVM 16. It also updates our host compiler for dist-x86_64-linux to LLVM 16. The reason for that is that Bolt from LLVM 15 is not capable of compiling LLVM 16 (llvm/llvm-project#61114).

LLVM 16.0.0 has been [released](https://discourse.llvm.org/t/llvm-16-0-0-release/69326) on March 18, while Rust 1.70 will become stable on June 1.

Tested images: `dist-x86_64-linux`, `dist-riscv64-linux` (alt), `dist-x86_64-illumos`, `dist-various-1`, `dist-various-2`, `dist-powerpc-linux`, `wasm32`, `armhf-gnu`
Tested images until the usual IPv6 failures: `test-various`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants