This is an archive of the discontinued LLVM Phabricator instance.

Differential D32214

[XRay][compiler-rt] Cleanup CFI/CFA annotations on trampolines
ClosedPublic

Authored by dberris on Apr 19 2017, 12:02 AM.

Details

Reviewers
kpw
pelikan
eugenis
Commits
rG9df8ef55384e: [XRay][compiler-rt] Cleanup CFI/CFA annotations on trampolines
rCRT300815: [XRay][compiler-rt] Cleanup CFI/CFA annotations on trampolines
rL300815: [XRay][compiler-rt] Cleanup CFI/CFA annotations on trampolines
Summary

This is a follow-up to D32202.

While the previous change (D32202) did fix the stack alignment issue, we
were still at a weird state in terms of the CFI/CFA directives (as the
offsets were wrong). This change cleans up the SAVE/RESTORE macros for
the trampoline, accounting the stack pointer adjustments with less
instructions and with some clearer math. We note that the offsets will
be different on the exit trampolines, because we don't typically 'call'
into this trampoline and we only ever jump into them (i.e. treated as a
tail call that's patched in at runtime).

Diff Detail

Event Timeline

dberris created this revision.Apr 19 2017, 12:02 AM
kpw accepted this revision.Apr 19 2017, 12:44 AM
kpw added inline comments.
lib/xray/xray_trampoline_x86_64.S
101

If the exit handler is jumped into, does the exit sled explicitly push the return address onto the stack (accounting for the extra 8 bytes compared to the subq insn above)?

This revision is now accepted and ready to land.Apr 19 2017, 12:44 AM
pelikan accepted this revision.Apr 19 2017, 6:00 PM

Third time does the charm?

lib/xray/xray_trampoline_x86_64.S
101

Either the sled (unpatched) or this trampoline (patched) will execute the RET instruction which will pop the address from the stack. It will happen precisely once and therefore offset 8 is correct. The push happens before the entry into the function, and has nothing to do with XRay.

dberris updated this revision to Diff 95869.Apr 19 2017, 6:47 PM
  • fixup: update with a comment explaining the stack alignment expectations
dberris marked 2 inline comments as done.Apr 19 2017, 6:49 PM

Updated the macro with a comment explaining the situation for stack alignment (taking @eugenis' advice in D32202).

dberris mentioned this in D32202: [XRay][compiler-rt] Fix up CFI annotations and stack alignment.Apr 19 2017, 6:52 PM
Closed by commit rL300815: [XRay][compiler-rt] Cleanup CFI/CFA annotations on trampolines (authored by dberris). · Explain WhyApr 19 2017, 8:38 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
xray/
30 lines

Diff 95691

lib/xray/xray_trampoline_x86_64.S

Show All 10 Lines
// //
// This implements the X86-specific assembler for the trampolines. // This implements the X86-specific assembler for the trampolines.
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "../builtins/assembly.h" #include "../builtins/assembly.h"
.macro SAVE_REGISTERS .macro SAVE_REGISTERS
subq $184, %rsp subq $192, %rsp
.cfi_def_cfa_offset 200
movq %rbp, 184(%rsp)
movupd %xmm0, 168(%rsp) movupd %xmm0, 168(%rsp)
movupd %xmm1, 152(%rsp) movupd %xmm1, 152(%rsp)
movupd %xmm2, 136(%rsp) movupd %xmm2, 136(%rsp)
movupd %xmm3, 120(%rsp) movupd %xmm3, 120(%rsp)
movupd %xmm4, 104(%rsp) movupd %xmm4, 104(%rsp)
movupd %xmm5, 88(%rsp) movupd %xmm5, 88(%rsp)
movupd %xmm6, 72(%rsp) movupd %xmm6, 72(%rsp)
movupd %xmm7, 56(%rsp) movupd %xmm7, 56(%rsp)
movq %rdi, 48(%rsp) movq %rdi, 48(%rsp)
movq %rax, 40(%rsp) movq %rax, 40(%rsp)
movq %rdx, 32(%rsp) movq %rdx, 32(%rsp)
movq %rsi, 24(%rsp) movq %rsi, 24(%rsp)
movq %rcx, 16(%rsp) movq %rcx, 16(%rsp)
movq %r8, 8(%rsp) movq %r8, 8(%rsp)
movq %r9, 0(%rsp) movq %r9, 0(%rsp)
.endm .endm
.macro RESTORE_REGISTERS .macro RESTORE_REGISTERS
movq 184(%rsp), %rbp
movupd 168(%rsp), %xmm0 movupd 168(%rsp), %xmm0
movupd 152(%rsp), %xmm1 movupd 152(%rsp), %xmm1
movupd 136(%rsp), %xmm2 movupd 136(%rsp), %xmm2
movupd 120(%rsp), %xmm3 movupd 120(%rsp), %xmm3
movupd 104(%rsp), %xmm4 movupd 104(%rsp), %xmm4
movupd 88(%rsp), %xmm5 movupd 88(%rsp), %xmm5
movupd 72(%rsp) , %xmm6 movupd 72(%rsp) , %xmm6
movupd 56(%rsp) , %xmm7 movupd 56(%rsp) , %xmm7
movq 48(%rsp), %rdi movq 48(%rsp), %rdi
movq 40(%rsp), %rax movq 40(%rsp), %rax
movq 32(%rsp), %rdx movq 32(%rsp), %rdx
movq 24(%rsp), %rsi movq 24(%rsp), %rsi
movq 16(%rsp), %rcx movq 16(%rsp), %rcx
movq 8(%rsp), %r8 movq 8(%rsp), %r8
movq 0(%rsp), %r9 movq 0(%rsp), %r9
addq $184, %rsp addq $192, %rsp
.cfi_def_cfa_offset 8
.endm .endm
.text .text
.file "xray_trampoline_x86.S" .file "xray_trampoline_x86.S"
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
.globl __xray_FunctionEntry .globl __xray_FunctionEntry
.align 16, 0x90 .align 16, 0x90
.type __xray_FunctionEntry,@function .type __xray_FunctionEntry,@function
__xray_FunctionEntry: __xray_FunctionEntry:
.cfi_startproc .cfi_startproc
pushq %rbp
.cfi_def_cfa_offset 16
SAVE_REGISTERS SAVE_REGISTERS
.cfi_def_cfa_offset 200
// This load has to be atomic, it's concurrent with __xray_patch(). // This load has to be atomic, it's concurrent with __xray_patch().
// On x86/amd64, a simple (type-aligned) MOV instruction is enough. // On x86/amd64, a simple (type-aligned) MOV instruction is enough.
movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax
testq %rax, %rax testq %rax, %rax
je .Ltmp0 je .Ltmp0
// The patched function prolog puts its xray_instr_map index into %r10d. // The patched function prolog puts its xray_instr_map index into %r10d.
movl %r10d, %edi movl %r10d, %edi
xor %esi,%esi xor %esi,%esi
callq *%rax callq *%rax
.Ltmp0: .Ltmp0:
RESTORE_REGISTERS RESTORE_REGISTERS
popq %rbp
retq retq
.Ltmp1: .Ltmp1:
.size __xray_FunctionEntry, .Ltmp1-__xray_FunctionEntry .size __xray_FunctionEntry, .Ltmp1-__xray_FunctionEntry
.cfi_endproc .cfi_endproc
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
.globl __xray_FunctionExit .globl __xray_FunctionExit
.align 16, 0x90 .align 16, 0x90
.type __xray_FunctionExit,@function .type __xray_FunctionExit,@function
__xray_FunctionExit: __xray_FunctionExit:
.cfi_startproc .cfi_startproc
// Save the important registers first. Since we're assuming that this // Save the important registers first. Since we're assuming that this
// function is only jumped into, we only preserve the registers for // function is only jumped into, we only preserve the registers for
// returning. // returning.
pushq %rbp subq $56, %rsp
.cfi_def_cfa_offset 16
subq $48, %rsp
.cfi_def_cfa_offset 64 .cfi_def_cfa_offset 64
kpwUnsubmitted
Done
ReplyInline Actions

If the exit handler is jumped into, does the exit sled explicitly push the return address onto the stack (accounting for the extra 8 bytes compared to the subq insn above)?

kpw: If the exit handler is jumped into, does the exit sled explicitly push the return address onto…
pelikanUnsubmitted
Done
ReplyInline Actions

Either the sled (unpatched) or this trampoline (patched) will execute the RET instruction which will pop the address from the stack. It will happen precisely once and therefore offset 8 is correct. The push happens before the entry into the function, and has nothing to do with XRay.

pelikan: Either the sled (unpatched) or this trampoline (patched) will execute the RET instruction which…
movq %rbp, 48(%rsp)
movupd %xmm0, 32(%rsp) movupd %xmm0, 32(%rsp)
movupd %xmm1, 16(%rsp) movupd %xmm1, 16(%rsp)
movq %rax, 8(%rsp) movq %rax, 8(%rsp)
movq %rdx, 0(%rsp) movq %rdx, 0(%rsp)
movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax
testq %rax,%rax testq %rax,%rax
je .Ltmp2 je .Ltmp2
movl %r10d, %edi movl %r10d, %edi
movl $1, %esi movl $1, %esi
callq *%rax callq *%rax
.Ltmp2: .Ltmp2:
// Restore the important registers. // Restore the important registers.
movq 48(%rsp), %rbp
movupd 32(%rsp), %xmm0 movupd 32(%rsp), %xmm0
movupd 16(%rsp), %xmm1 movupd 16(%rsp), %xmm1
movq 8(%rsp), %rax movq 8(%rsp), %rax
movq 0(%rsp), %rdx movq 0(%rsp), %rdx
addq $48, %rsp addq $56, %rsp
popq %rbp .cfi_def_cfa_offset 8
retq retq
.Ltmp3: .Ltmp3:
.size __xray_FunctionExit, .Ltmp3-__xray_FunctionExit .size __xray_FunctionExit, .Ltmp3-__xray_FunctionExit
.cfi_endproc .cfi_endproc
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
.global __xray_FunctionTailExit .global __xray_FunctionTailExit
.align 16, 0x90 .align 16, 0x90
.type __xray_FunctionTailExit,@function .type __xray_FunctionTailExit,@function
__xray_FunctionTailExit: __xray_FunctionTailExit:
.cfi_startproc .cfi_startproc
// Save the important registers as in the entry trampoline, but indicate that // Save the important registers as in the entry trampoline, but indicate that
// this is an exit. In the future, we will introduce a new entry type that // this is an exit. In the future, we will introduce a new entry type that
// differentiates between a normal exit and a tail exit, but we'd have to do // differentiates between a normal exit and a tail exit, but we'd have to do
// this and increment the version number for the header. // this and increment the version number for the header.
pushq %rbp
.cfi_def_cfa_offset 16
SAVE_REGISTERS SAVE_REGISTERS
.cfi_def_cfa_offset 200
movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax
testq %rax,%rax testq %rax,%rax
je .Ltmp4 je .Ltmp4
movl %r10d, %edi movl %r10d, %edi
movl $1, %esi movl $1, %esi
callq *%rax callq *%rax
.Ltmp4: .Ltmp4:
RESTORE_REGISTERS RESTORE_REGISTERS
popq %rbp
retq retq
.Ltmp5: .Ltmp5:
.size __xray_FunctionTailExit, .Ltmp5-__xray_FunctionTailExit .size __xray_FunctionTailExit, .Ltmp5-__xray_FunctionTailExit
.cfi_endproc .cfi_endproc
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
.globl __xray_ArgLoggerEntry .globl __xray_ArgLoggerEntry
.align 16, 0x90 .align 16, 0x90
.type __xray_ArgLoggerEntry,@function .type __xray_ArgLoggerEntry,@function
__xray_ArgLoggerEntry: __xray_ArgLoggerEntry:
.cfi_startproc .cfi_startproc
pushq %rbp
.cfi_def_cfa_offset 16
SAVE_REGISTERS SAVE_REGISTERS
.cfi_def_cfa_offset 200
// Again, these function pointer loads must be atomic; MOV is fine. // Again, these function pointer loads must be atomic; MOV is fine.
movq _ZN6__xray13XRayArgLoggerE(%rip), %rax movq _ZN6__xray13XRayArgLoggerE(%rip), %rax
testq %rax, %rax testq %rax, %rax
jne .Larg1entryLog jne .Larg1entryLog
// If [arg1 logging handler] not set, defer to no-arg logging. // If [arg1 logging handler] not set, defer to no-arg logging.
movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax movq _ZN6__xray19XRayPatchedFunctionE(%rip), %rax
testq %rax, %rax testq %rax, %rax
je .Larg1entryFail je .Larg1entryFail
.Larg1entryLog: .Larg1entryLog:
movq %rdi, %rdx // first argument will become the third movq %rdi, %rdx // first argument will become the third
xorq %rsi, %rsi // XRayEntryType::ENTRY into the second xorq %rsi, %rsi // XRayEntryType::ENTRY into the second
movl %r10d, %edi // 32-bit function ID becomes the first movl %r10d, %edi // 32-bit function ID becomes the first
callq *%rax callq *%rax
.Larg1entryFail: .Larg1entryFail:
RESTORE_REGISTERS RESTORE_REGISTERS
popq %rbp
retq retq
.Larg1entryEnd: .Larg1entryEnd:
.size __xray_ArgLoggerEntry, .Larg1entryEnd-__xray_ArgLoggerEntry .size __xray_ArgLoggerEntry, .Larg1entryEnd-__xray_ArgLoggerEntry
.cfi_endproc .cfi_endproc
NO_EXEC_STACK_DIRECTIVE NO_EXEC_STACK_DIRECTIVE