This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt][x86_64] Fix CFI directives in assembly trampolines
ClosedPublic

Authored by EliaGeretto on Feb 16 2021, 6:55 AM.

Details

Summary

This patch modifies the x86_64 XRay trampolines to fix the CFI information
generated by the assembler. One of the main issues in correcting the CFI
directives is the ALIGNED_CALL_RAX macro, which makes the CFA dependent on
the alignment of the stack. However, this macro is not really necessary because
some additional assumptions can be made on the alignment of the stack when the
trampolines are called. The code has been written as if the stack is guaranteed
to be 8-bytes aligned; however, it is instead guaranteed to be misaligned by 8
bytes with respect to a 16-bytes alignment. For this reason, always moving the
stack pointer by 8 bytes is sufficient to restore the appropriate alignment.

Trampolines that are called from within a function as a result of the builtins
__xray_typedevent and __xray_customevent are necessarely called with the
stack properly aligned so, in this case too, ALIGNED_CALL_RAX can be
eliminated.

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=49060.

Diff Detail

Event Timeline

EliaGeretto created this revision.Feb 16 2021, 6:55 AM
EliaGeretto requested review of this revision.Feb 16 2021, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 6:55 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Feb 16 2021, 10:02 AM
compiler-rt/lib/xray/xray_trampoline_x86_64.S
30

FIX_STACK and UNFIX_STACK are probably confusing names. Can we just rename them to something something with PUSH/POP in the names?

35

Nit: append period to complete sentences in comments.

EliaGeretto added inline comments.Feb 16 2021, 10:15 AM
compiler-rt/lib/xray/xray_trampoline_x86_64.S
30

Right, the current names are definitely confusing. Since it has to do with alignment, could it be something like ALIGN_STACK_TO_16_BYTES and RESTORE_PREVIOUS_ALIGNMENT? That should also clarify what I am trying to do a bit better.

dberris added inline comments.Feb 16 2021, 6:19 PM
compiler-rt/lib/xray/xray_trampoline_x86_64.S
30

SAVE_AND_ALIGN_STACK_16B and RESTORE_STACK_ALIGNMENT?

I went with ALIGN_STACK_16B since I am not really saving anything, just modifying the stack pointer. The rest of the changes are as requested.

EliaGeretto marked 3 inline comments as done.Feb 17 2021, 1:38 AM
MaskRay accepted this revision.Mar 1 2021, 11:13 AM

LGTM.

This revision is now accepted and ready to land.Mar 1 2021, 11:13 AM

@MaskRay Could you please push this on my behalf? I do not have push access. Also, this fix should be small and self contained enough to be considered for inclusion in the LLVM 12 branch. Is it possible?

This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Mar 3 2021, 1:07 PM

This change seems to have broken the build on Darwin, would it be possible to look into it or revert the change?

/opt/s/w/ir/x/w/staging/llvm_build/./bin/clang -target x86_64-apple-darwin19.6.0 --sysroot=/opt/s/w/ir/cache/macos_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -DXRAY_HAS_EXCEPTIONS=1 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__func__=__FUNCTION__ -I/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/.. -I/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/../../include -fPIC -O3 -DNDEBUG -arch x86_64 -arch x86_64h -isysroot /opt/s/w/ir/cache/macos_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk  -stdlib=libc++ -mmacosx-version-min=10.7 -isysroot /opt/s/w/ir/cache/macos_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -O3 -stdlib=libc++ -MD -MT compiler-rt/lib/xray/CMakeFiles/RTXray.osx.dir/xray_trampoline_x86_64.S.o -MF compiler-rt/lib/xray/CMakeFiles/RTXray.osx.dir/xray_trampoline_x86_64.S.o.d -o compiler-rt/lib/xray/CMakeFiles/RTXray.osx.dir/xray_trampoline_x86_64.S.o -c /opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S
<instantiation>:1:6: error: unknown token in expression
subq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:112:2: note: while in macro instantiation
 ALIGN_STACK_16B
 ^
<instantiation>:1:6: error: unknown token in expression
addq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:128:2: note: while in macro instantiation
 RESTORE_STACK_ALIGNMENT
 ^
<instantiation>:1:6: error: unknown token in expression
subq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:143:2: note: while in macro instantiation
 ALIGN_STACK_16B
 ^
<instantiation>:1:6: error: unknown token in expression
addq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:173:2: note: while in macro instantiation
 RESTORE_STACK_ALIGNMENT
 ^
<instantiation>:1:6: error: unknown token in expression
subq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:188:2: note: while in macro instantiation
 ALIGN_STACK_16B
 ^
<instantiation>:1:6: error: unknown token in expression
addq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:201:2: note: while in macro instantiation
 RESTORE_STACK_ALIGNMENT
 ^
<instantiation>:1:6: error: unknown token in expression
subq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:216:2: note: while in macro instantiation
 ALIGN_STACK_16B
 ^
<instantiation>:1:6: error: unknown token in expression
addq , %rsp
     ^
/opt/s/w/ir/x/w/llvm-project/compiler-rt/lib/xray/xray_trampoline_x86_64.S:244:2: note: while in macro instantiation
 RESTORE_STACK_ALIGNMENT
 ^

Unfortunately, I have no way to test this on Darwin. It would seem that the $8 in ALIGN_STACK_16B and RESTORE_STACK_ALIGNMENT is expanded to an empty string for some reason. Do you have any idea why this would happen on your platform? I have also tried with clang on Linux (I was testing with gcc before), but this expansion does not happen. The $8 remains a $8. Also, it is unclear to me why this does not happen with subq $240, %rsp as well, for example.

Unfortunately, I have no way to test this on Darwin. It would seem that the $8 in ALIGN_STACK_16B and RESTORE_STACK_ALIGNMENT is expanded to an empty string for some reason. Do you have any idea why this would happen on your platform? I have also tried with clang on Linux (I was testing with gcc before), but this expansion does not happen. The $8 remains a $8. Also, it is unclear to me why this does not happen with subq $240, %rsp as well, for example.

Given that this is still failing, would be it OK to revert the change for now while we keep debugging this?

Sure, I cannot do this myself though. I do not have push access.

The problem is the following:

$d (where d is a single decimal digit, 0 through 9) represents each argument—there can be at most 10 arguments. $n is replaced by the actual number of arguments the macro is invoked with.

(OS X Assembler Reference)

I will define the .macro differently then, as soon as I am able to figure out how one is supposed to specify integer constants inside .macro-s.

I have modified the two macros to use double-dollar constants when on Darwin. This should fix the build issues that were reported by @phosek.

EliaGeretto reopened this revision.Mar 5 2021, 6:56 AM
This revision is now accepted and ready to land.Mar 5 2021, 6:56 AM
EliaGeretto requested review of this revision.Mar 5 2021, 6:58 AM

As previously stated, I modified the patch so that it supports Darwin as well. I am thus requesting a new review. It took me a while to figure out how to do this properly in Differential.

MaskRay added inline comments.Mar 5 2021, 3:30 PM
compiler-rt/lib/xray/xray_trampoline_x86_64.S
26

Use #if !defined(__APPLE__) (or #ifdef) to avoid negation in conditions.

MaskRay added inline comments.Mar 5 2021, 3:37 PM
compiler-rt/lib/xray/xray_trampoline_x86_64.S
34

Should Darwin assembly use addq 8, %rsp

You can test with llvm-mc -filetype=obj -triple=x86_64-apple-darwin a.s but that is after preprocessing.

EliaGeretto marked an inline comment as done.Mar 6 2021, 4:17 AM
EliaGeretto added inline comments.
compiler-rt/lib/xray/xray_trampoline_x86_64.S
26

I used the !defined(__APPLE__) to match the other conditions in the file. Anyway, I will flip this one, no problem.

34

No, I checked. The double dollar is the correct solution (also what is used in XNU). Both are valid syntax, but with $$8 you get:

0: 48 83 c4 08                  	add	rsp, 8

With just the 8, instead, you get:

0: 48 03 24 25 08 00 00 00      	add	rsp, qword ptr [8]

I imagine referring to macro arguments with $n is quite a source of trouble, definitely not the best design choice.

EliaGeretto marked an inline comment as done.Mar 6 2021, 4:19 AM

As requested, I have flipped the #if !defined(__APPLE__) into #if defined(__APPLE__).

EliaGeretto marked an inline comment as done.Mar 6 2021, 10:04 AM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 6 2021, 10:38 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.