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

[EliaGeretto]

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.

[MaskRay]

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]

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]

compiler-rt/lib/xray/xray_trampoline_x86_64.S

30

SAVE_AND_ALIGN_STACK_16B and RESTORE_STACK_ALIGNMENT?

[EliaGeretto]

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.

[MaskRay]

LGTM.

[EliaGeretto]

@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?

[phosek]

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
 ^

[EliaGeretto]

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.

[phosek]

In D96785#2602645, @EliaGeretto wrote:

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?

[EliaGeretto]

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

[EliaGeretto]

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.

[EliaGeretto]

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]

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]

compiler-rt/lib/xray/xray_trampoline_x86_64.S

26

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

[MaskRay]

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]

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]

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

compiler-rt/lib/xray/xray_trampoline_x86_64.S

 //
 // This implements the X86-specific assembler for the trampolines.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../builtins/assembly.h"
 #include "../sanitizer_common/sanitizer_asm.h"
 
+// XRay trampolines which are not produced by intrinsics are not System V AMD64
+// ABI compliant because they are called with a stack that is always misaligned
+// by 8 bytes with respect to a 16 bytes alignment. This is because they are
+// called immediately after the call to, or immediately before returning from,
+// the function being instrumented. This saves space in the patch point, but
+// misaligns the stack by 8 bytes.
+
+.macro ALIGN_STACK_16B
+  subq  $8, %rsp
+  CFI_ADJUST_CFA_OFFSET(8)
+.endm
 
+.macro RESTORE_STACK_ALIGNMENT
+  addq  $8, %rsp
+  CFI_ADJUST_CFA_OFFSET(-8)
+.endm
 
+// This macro should keep the stack aligned to 16 bytes.
 .macro SAVE_REGISTERS
   pushfq
+  CFI_ADJUST_CFA_OFFSET(8)
   subq $240, %rsp
-  CFI_DEF_CFA_OFFSET(248)
+  CFI_ADJUST_CFA_OFFSET(240)
   movq %rbp, 232(%rsp)
   movupd  %xmm0, 216(%rsp)
   movupd  %xmm1, 200(%rsp)
   movupd  %xmm2, 184(%rsp)
   movupd  %xmm3, 168(%rsp)
   movupd  %xmm4, 152(%rsp)
   movupd  %xmm5, 136(%rsp)
   movupd  %xmm6, 120(%rsp)
   movupd  %xmm7, 104(%rsp)
   movq  %rdi, 96(%rsp)
   movq  %rax, 88(%rsp)
   movq  %rdx, 80(%rsp)
   movq  %rsi, 72(%rsp)
   movq  %rcx, 64(%rsp)
   movq  %r8, 56(%rsp)
   movq  %r9, 48(%rsp)
   movq  %r10, 40(%rsp)
   movq  %r11, 32(%rsp)
   movq  %r12, 24(%rsp)
   movq  %r13, 16(%rsp)
   movq  %r14, 8(%rsp)
   movq  %r15, 0(%rsp)
 .endm
 
+// This macro should keep the stack aligned to 16 bytes.
 .macro RESTORE_REGISTERS
   movq  232(%rsp), %rbp
   movupd  216(%rsp), %xmm0
   movupd  200(%rsp), %xmm1
   movupd  184(%rsp), %xmm2
   movupd  168(%rsp), %xmm3
   movupd  152(%rsp), %xmm4
   movupd  136(%rsp), %xmm5
   movupd  120(%rsp) , %xmm6
   movupd  104(%rsp) , %xmm7
   movq  96(%rsp), %rdi
   movq  88(%rsp), %rax
   movq  80(%rsp), %rdx
   movq  72(%rsp), %rsi
   movq  64(%rsp), %rcx
   movq  56(%rsp), %r8
   movq  48(%rsp), %r9
   movq  40(%rsp), %r10
   movq  32(%rsp), %r11
   movq  24(%rsp), %r12
   movq  16(%rsp), %r13
   movq  8(%rsp), %r14
   movq  0(%rsp), %r15
   addq  $240, %rsp
+  CFI_ADJUST_CFA_OFFSET(-240)
   popfq
-  CFI_DEF_CFA_OFFSET(8)
-.endm
-
-.macro ALIGNED_CALL_RAX
-  // Call the logging handler, after aligning the stack to a 16-byte boundary.
-  // The approach we're taking here uses additional stack space to stash the
-  // stack pointer twice before aligning the pointer to 16-bytes. If the stack
-  // was 8-byte aligned, it will become 16-byte aligned -- when restoring the
-  // pointer, we can always look -8 bytes from the current position to get
-  // either of the values we've stashed in the first place.
-  pushq %rsp
-  pushq (%rsp)
-  andq $-0x10, %rsp
-  callq *%rax
-  movq 8(%rsp), %rsp
+  CFI_ADJUST_CFA_OFFSET(-8)
 .endm
 
   .text
 #if !defined(__APPLE__)
   .section .text
   .file "xray_trampoline_x86.S"
 #else
   .section __TEXT,__text
 #endif
 
 //===----------------------------------------------------------------------===//
 
   .globl ASM_SYMBOL(__xray_FunctionEntry)
   ASM_HIDDEN(__xray_FunctionEntry)
   .align 16, 0x90
   ASM_TYPE_FUNCTION(__xray_FunctionEntry)
 # LLVM-MCA-BEGIN __xray_FunctionEntry
 ASM_SYMBOL(__xray_FunctionEntry):
   CFI_STARTPROC
+  ALIGN_STACK_16B
   SAVE_REGISTERS
 
   // This load has to be atomic, it's concurrent with __xray_patch().
   // On x86/amd64, a simple (type-aligned) MOV instruction is enough.
   movq  ASM_SYMBOL(_ZN6__xray19XRayPatchedFunctionE)(%rip), %rax
   testq %rax, %rax
   je  .Ltmp0
 
   // The patched function prologue puts its xray_instr_map index into %r10d.
   movl  %r10d, %edi
   xor %esi,%esi
-  ALIGNED_CALL_RAX
+  callq *%rax
 
 .Ltmp0:
   RESTORE_REGISTERS
+  RESTORE_STACK_ALIGNMENT
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_FunctionEntry)
   CFI_ENDPROC
 
 //===----------------------------------------------------------------------===//
 
   .globl ASM_SYMBOL(__xray_FunctionExit)
   ASM_HIDDEN(__xray_FunctionExit)
   .align 16, 0x90
   ASM_TYPE_FUNCTION(__xray_FunctionExit)
 # LLVM-MCA-BEGIN __xray_FunctionExit
 ASM_SYMBOL(__xray_FunctionExit):
   CFI_STARTPROC
+  ALIGN_STACK_16B
+
   // Save the important registers first. Since we're assuming that this
   // function is only jumped into, we only preserve the registers for
   // returning.
-  subq  $56, %rsp
-  CFI_DEF_CFA_OFFSET(64)
+  subq  $64, %rsp
+  CFI_ADJUST_CFA_OFFSET(64)
   movq  %rbp, 48(%rsp)
   movupd  %xmm0, 32(%rsp)
   movupd  %xmm1, 16(%rsp)
   movq  %rax, 8(%rsp)
   movq  %rdx, 0(%rsp)
   movq  ASM_SYMBOL(_ZN6__xray19XRayPatchedFunctionE)(%rip), %rax
   testq %rax,%rax
   je  .Ltmp2
 
   movl  %r10d, %edi
   movl  $1, %esi
-  ALIGNED_CALL_RAX
+  callq *%rax
 
 .Ltmp2:
   // Restore the important registers.
   movq  48(%rsp), %rbp
   movupd  32(%rsp), %xmm0
   movupd  16(%rsp), %xmm1
   movq  8(%rsp), %rax
   movq  0(%rsp), %rdx
-  addq  $56, %rsp
-  CFI_DEF_CFA_OFFSET(8)
+  addq  $64, %rsp
+  CFI_ADJUST_CFA_OFFSET(-64)
+
+  RESTORE_STACK_ALIGNMENT
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_FunctionExit)
   CFI_ENDPROC
 
 //===----------------------------------------------------------------------===//
 
   .globl ASM_SYMBOL(__xray_FunctionTailExit)
   ASM_HIDDEN(__xray_FunctionTailExit)
   .align 16, 0x90
   ASM_TYPE_FUNCTION(__xray_FunctionTailExit)
 # LLVM-MCA-BEGIN __xray_FunctionTailExit
 ASM_SYMBOL(__xray_FunctionTailExit):
   CFI_STARTPROC
+  ALIGN_STACK_16B
   SAVE_REGISTERS
 
   movq  ASM_SYMBOL(_ZN6__xray19XRayPatchedFunctionE)(%rip), %rax
   testq %rax,%rax
   je  .Ltmp4
 
   movl  %r10d, %edi
   movl  $2, %esi
-
-  ALIGNED_CALL_RAX
+  callq *%rax
 
 .Ltmp4:
   RESTORE_REGISTERS
+  RESTORE_STACK_ALIGNMENT
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_FunctionTailExit)
   CFI_ENDPROC
 
 //===----------------------------------------------------------------------===//
 
   .globl ASM_SYMBOL(__xray_ArgLoggerEntry)
   ASM_HIDDEN(__xray_ArgLoggerEntry)
   .align 16, 0x90
   ASM_TYPE_FUNCTION(__xray_ArgLoggerEntry)
 # LLVM-MCA-BEGIN __xray_ArgLoggerEntry
 ASM_SYMBOL(__xray_ArgLoggerEntry):
   CFI_STARTPROC
+  ALIGN_STACK_16B
   SAVE_REGISTERS
 
   // Again, these function pointer loads must be atomic; MOV is fine.
   movq  ASM_SYMBOL(_ZN6__xray13XRayArgLoggerE)(%rip), %rax
   testq %rax, %rax
   jne .Larg1entryLog
 
   // If [arg1 logging handler] not set, defer to no-arg logging.
   movq  ASM_SYMBOL(_ZN6__xray19XRayPatchedFunctionE)(%rip), %rax
   testq %rax, %rax
   je  .Larg1entryFail
 
 .Larg1entryLog:
 
   // First argument will become the third
   movq  %rdi, %rdx
 
   // XRayEntryType::LOG_ARGS_ENTRY into the second
   mov $0x3, %esi
 
   // 32-bit function ID becomes the first
   movl  %r10d, %edi
-  ALIGNED_CALL_RAX
+
+  callq *%rax
 
 .Larg1entryFail:
   RESTORE_REGISTERS
+  RESTORE_STACK_ALIGNMENT
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_ArgLoggerEntry)
   CFI_ENDPROC
 
 //===----------------------------------------------------------------------===//
 
   .global ASM_SYMBOL(__xray_CustomEvent)
   ASM_HIDDEN(__xray_CustomEvent)
   .align 16, 0x90
   ASM_TYPE_FUNCTION(__xray_CustomEvent)
 # LLVM-MCA-BEGIN __xray_CustomEvent
 ASM_SYMBOL(__xray_CustomEvent):
   CFI_STARTPROC
   SAVE_REGISTERS
 
   // We take two arguments to this trampoline, which should be in rdi  and rsi
   // already.
   movq ASM_SYMBOL(_ZN6__xray22XRayPatchedCustomEventE)(%rip), %rax
   testq %rax,%rax
   je .LcustomEventCleanup
 
-  ALIGNED_CALL_RAX
+  callq *%rax
 
 .LcustomEventCleanup:
   RESTORE_REGISTERS
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_CustomEvent)
   CFI_ENDPROC
 
ASM_SYMBOL(__xray_TypedEvent):
   SAVE_REGISTERS
 
   // We pass three arguments to this trampoline, which should be in rdi, rsi
   // and rdx without our intervention.
   movq ASM_SYMBOL(_ZN6__xray21XRayPatchedTypedEventE)(%rip), %rax
   testq %rax,%rax
   je .LtypedEventCleanup
 
-  ALIGNED_CALL_RAX
+  callq *%rax
 
 .LtypedEventCleanup:
   RESTORE_REGISTERS
   retq
 # LLVM-MCA-END
   ASM_SIZE(__xray_TypedEvent)
   CFI_ENDPROC
 
 //===----------------------------------------------------------------------===//
 
 NO_EXEC_STACK_DIRECTIVE