[PATCH] D70413: [musttail] Don't forward incoming registers over call site registers

[Reid Kleckner via Phabricator via llvm-commits]

rnk created this revision.
rnk added reviewers: ajpaverd, efriedma, hans.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

On Win64, the original indirect call is replaced with a new call that
passes the original call target in RAX. We also conservatively attempted
to forward the incoming value of AL from the prologue through to the
call. This resulted in a sequence of two COPYs into RAX and AL, and the
second copy corrupts the function pointer. The verifier check that
musttail callees have the same convention is meant to ensure that the
prologue and epilogue have the same sets of incoming and outgoing
registers, but control flow guard uses a bundle to work around that
rule.

For use cases like this, I think it's best to assume that values at the
call site should not be overwritten with registers preserved in the
prologue.

Fixes PR44049


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70413

Files:
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/X86/cfguard-checks.ll


Index: llvm/test/CodeGen/X86/cfguard-checks.ll
===================================================================
--- llvm/test/CodeGen/X86/cfguard-checks.ll
+++ llvm/test/CodeGen/X86/cfguard-checks.ll
@@ -176,6 +176,39 @@
   ; X64-NOT:   callq
 }
 
+%struct.Foo = type { i32 (%struct.Foo*)** }
+
+; Test that Control Flow Guard checks are correctly added for variadic musttail
+; calls. These are used for MS C++ ABI virtual member pointer thunks.
+; PR44049
+define i32 @vmptr_thunk(%struct.Foo* inreg %p) {
+entry:
+  %vptr.addr = getelementptr inbounds %struct.Foo, %struct.Foo* %p, i32 0, i32 0
+  %vptr = load i32 (%struct.Foo*)**, i32 (%struct.Foo*)*** %vptr.addr
+  %slot = getelementptr inbounds i32 (%struct.Foo*)*, i32 (%struct.Foo*)** %vptr, i32 1
+  %vmethod = load i32 (%struct.Foo*)*, i32 (%struct.Foo*)** %slot
+  %rv = musttail call i32 %vmethod(%struct.Foo* inreg %p)
+  ret i32 %rv
+
+  ; On i686, the call to __guard_check_icall_fptr should come immediately before the call to the target function.
+  ; X32-LABEL: _vmptr_thunk:
+  ; X32:       movl %eax, %esi
+  ; X32:       movl (%eax), %eax
+  ; X32:       movl 4(%eax), %ecx
+  ; X32:       calll *___guard_check_icall_fptr
+  ; X32:       movl %esi, %eax
+  ; X32:       jmpl       *%ecx                  # TAILCALL
+  ; X32-NOT:   calll
+
+  ; Use NEXT here because we previously had an extra instruction in this sequence.
+  ; X64-LABEL: vmptr_thunk:
+  ; X64:            movq (%rcx), %rax
+  ; X64-NEXT:       movq 8(%rax), %rax
+  ; X64-NEXT:       movq __guard_dispatch_icall_fptr(%rip), %rdx
+  ; X64-NEXT:       addq $40, %rsp
+  ; X64-NEXT:       rex64 jmpq *%rdx            # TAILCALL
+  ; X64-NOT:   callq
+}
 
 ; Test that longjmp targets have public labels and are included in the .gljmp section.
 %struct._SETJMP_FLOAT128 = type { [2 x i64] }
Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -3894,10 +3894,16 @@
   }
 
   if (isVarArg && IsMustTail) {
+    // Forward registers for musttail calls. Don't forward a register if it's
+    // been allocated for some other purpose. This arises when control flow
+    // guard passes an extra indirect call target in RAX, which conflicts with
+    // forwarding the original incoming value of AL. In these cases, the value
+    // passed at the call site wins.
     const auto &Forwards = X86Info->getForwardedMustTailRegParms();
     for (const auto &F : Forwards) {
       SDValue Val = DAG.getCopyFromReg(Chain, dl, F.VReg, F.VT);
-      RegsToPass.push_back(std::make_pair(unsigned(F.PReg), Val));
+      if (!CCInfo.isAllocated(F.PReg))
+        RegsToPass.push_back(std::make_pair(unsigned(F.PReg), Val));
     }
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70413.229941.patch
Type: text/x-patch
Size: 2818 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191118/ce21ac43/attachment.bin>