IRGen: Move vtable load after argument evaluation.

This change reduces the live range of the loaded function pointer,
resulting in a slight code size decrease (~10KB in clang), and also
improves the security of CFI for virtual calls by making it less
likely that the function pointer will be spilled, and ensuring that
it is not spilled across a function call boundary.

Fixes PR35353.

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

llvm-svn: 324286

6 files changed, 112 insertions, 78 deletions

diff --git a/clang/lib/CodeGen/CGCXXABI.h b/clang/lib/CodeGen/CGCXXABI.h
index 83426dc3a03..56adcbc5750 100644
--- a/clang/lib/CodeGen/CGCXXABI.h
+++ b/clang/lib/CodeGen/CGCXXABI.h
@@ -413,11 +413,10 @@ public:
                                                 CharUnits VPtrOffset) = 0;
 
   /// Build a virtual function pointer in the ABI-specific way.
-  virtual CGCallee getVirtualFunctionPointer(CodeGenFunction &CGF,
-                                             GlobalDecl GD,
-                                             Address This,
-                                             llvm::Type *Ty,
-                                             SourceLocation Loc) = 0;
+  virtual llvm::Value *getVirtualFunctionPointer(CodeGenFunction &CGF,
+                                                 GlobalDecl GD, Address This,
+                                                 llvm::Type *Ty,
+                                                 SourceLocation Loc) = 0;
 
   /// Emit the ABI-specific virtual destructor call.
   virtual llvm::Value *
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index d8b8e78ebf2..0f222ae21ab 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3728,7 +3728,7 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
                                  SourceLocation Loc) {
   // FIXME: We no longer need the types from CallArgs; lift up and simplify.
 
-  assert(Callee.isOrdinary());
+  assert(Callee.isOrdinary() || Callee.isVirtual());
 
   // Handle struct-return functions by passing a pointer to the
   // location that we would like to return into.
@@ -4052,7 +4052,14 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
     }
   }
 
-  llvm::Value *CalleePtr = Callee.getFunctionPointer();
+  llvm::Value *CalleePtr;
+  if (Callee.isVirtual()) {
+    const CallExpr *CE = Callee.getVirtualCallExpr();
+    CalleePtr = CGM.getCXXABI().getVirtualFunctionPointer(
+        *this, Callee.getVirtualMethodDecl(), Callee.getThisAddress(),
+        Callee.getFunctionType(), CE ? CE->getLocStart() : SourceLocation());
+  } else
+    CalleePtr = Callee.getFunctionPointer();
 
   // If we're using inalloca, set up that argument.
   if (ArgMemory.isValid()) {
diff --git a/clang/lib/CodeGen/CGCall.h b/clang/lib/CodeGen/CGCall.h
index 7e10407fc31..6b23a81fa97 100644
--- a/clang/lib/CodeGen/CGCall.h
+++ b/clang/lib/CodeGen/CGCall.h
@@ -18,6 +18,7 @@
 #include "CGValue.h"
 #include "EHScopeStack.h"
 #include "clang/AST/CanonicalType.h"
+#include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 
@@ -68,8 +69,9 @@ public:
       Invalid,
       Builtin,
       PseudoDestructor,
+      Virtual,
 
-      Last = PseudoDestructor
+      Last = Virtual
     };
 
     struct BuiltinInfoStorage {
@@ -79,12 +81,19 @@ public:
     struct PseudoDestructorInfoStorage {
       const CXXPseudoDestructorExpr *Expr;
     };
+    struct VirtualInfoStorage {
+      const CallExpr *CE;
+      GlobalDecl MD;
+      Address Addr;
+      llvm::FunctionType *FTy;
+    };
 
     SpecialKind KindOrFunctionPointer;
     union {
       CGCalleeInfo AbstractInfo;
       BuiltinInfoStorage BuiltinInfo;
       PseudoDestructorInfoStorage PseudoDestructorInfo;
+      VirtualInfoStorage VirtualInfo;
     };
 
     explicit CGCallee(SpecialKind kind) : KindOrFunctionPointer(kind) {}
@@ -127,6 +136,16 @@ public:
       return CGCallee(abstractInfo, functionPtr);
     }
 
+    static CGCallee forVirtual(const CallExpr *CE, GlobalDecl MD, Address Addr,
+                               llvm::FunctionType *FTy) {
+      CGCallee result(SpecialKind::Virtual);
+      result.VirtualInfo.CE = CE;
+      result.VirtualInfo.MD = MD;
+      result.VirtualInfo.Addr = Addr;
+      result.VirtualInfo.FTy = FTy;
+      return result;
+    }
+
     bool isBuiltin() const {
       return KindOrFunctionPointer == SpecialKind::Builtin;
     }
@@ -150,7 +169,9 @@ public:
     bool isOrdinary() const {
       return uintptr_t(KindOrFunctionPointer) > uintptr_t(SpecialKind::Last);
     }
-    const CGCalleeInfo &getAbstractInfo() const {
+    CGCalleeInfo getAbstractInfo() const {
+      if (isVirtual())
+        return VirtualInfo.MD.getDecl();
       assert(isOrdinary());
       return AbstractInfo;
     }
@@ -158,14 +179,33 @@ public:
       assert(isOrdinary());
       return reinterpret_cast<llvm::Value*>(uintptr_t(KindOrFunctionPointer));
     }
-    llvm::FunctionType *getFunctionType() const {
-      return cast<llvm::FunctionType>(
-                    getFunctionPointer()->getType()->getPointerElementType());
-    }
     void setFunctionPointer(llvm::Value *functionPtr) {
       assert(isOrdinary());
       KindOrFunctionPointer = SpecialKind(uintptr_t(functionPtr));
     }
+
+    bool isVirtual() const {
+      return KindOrFunctionPointer == SpecialKind::Virtual;
+    }
+    const CallExpr *getVirtualCallExpr() const {
+      assert(isVirtual());
+      return VirtualInfo.CE;
+    }
+    GlobalDecl getVirtualMethodDecl() const {
+      assert(isVirtual());
+      return VirtualInfo.MD;
+    }
+    Address getThisAddress() const {
+      assert(isVirtual());
+      return VirtualInfo.Addr;
+    }
+
+    llvm::FunctionType *getFunctionType() const {
+      if (isVirtual())
+        return VirtualInfo.FTy;
+      return cast<llvm::FunctionType>(
+          getFunctionPointer()->getType()->getPointerElementType());
+    }
   };
 
   struct CallArg {
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 9a18751fc25..9f70ef3dcd5 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -371,9 +371,7 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
                   CGM.GetAddrOfFunction(GlobalDecl(Ctor, Ctor_Complete), Ty),
                                  Ctor);
   } else if (UseVirtualCall) {
-    Callee = CGM.getCXXABI().getVirtualFunctionPointer(*this, MD,
-                                                       This.getAddress(), Ty,
-                                                       CE->getLocStart());
+    Callee = CGCallee::forVirtual(CE, MD, This.getAddress(), Ty);
   } else {
     if (SanOpts.has(SanitizerKind::CFINVCall) &&
         MD->getParent()->isDynamicClass()) {
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 8963fc17b0c..2792636c7fe 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -280,9 +280,9 @@ public:
   llvm::GlobalVariable *getAddrOfVTable(const CXXRecordDecl *RD,
                                         CharUnits VPtrOffset) override;
 
-  CGCallee getVirtualFunctionPointer(CodeGenFunction &CGF, GlobalDecl GD,
-                                     Address This, llvm::Type *Ty,
-                                     SourceLocation Loc) override;
+  llvm::Value *getVirtualFunctionPointer(CodeGenFunction &CGF, GlobalDecl GD,
+                                         Address This, llvm::Type *Ty,
+                                         SourceLocation Loc) override;
 
   llvm::Value *EmitVirtualDestructorCall(CodeGenFunction &CGF,
                                          const CXXDestructorDecl *Dtor,
@@ -1651,47 +1651,42 @@ llvm::GlobalVariable *ItaniumCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
   return VTable;
 }
 
-CGCallee ItaniumCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
-                                                  GlobalDecl GD,
-                                                  Address This,
-                                                  llvm::Type *Ty,
-                                                  SourceLocation Loc) {
+llvm::Value *ItaniumCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
+                                                      GlobalDecl GD,
+                                                      Address This,
+                                                      llvm::Type *Ty,
+                                                      SourceLocation Loc) {
   GD = GD.getCanonicalDecl();
   Ty = Ty->getPointerTo()->getPointerTo();
   auto *MethodDecl = cast<CXXMethodDecl>(GD.getDecl());
   llvm::Value *VTable = CGF.GetVTablePtr(This, Ty, MethodDecl->getParent());
 
   uint64_t VTableIndex = CGM.getItaniumVTableContext().getMethodVTableIndex(GD);
-  llvm::Value *VFunc;
-  if (CGF.ShouldEmitVTableTypeCheckedLoad(MethodDecl->getParent())) {
-    VFunc = CGF.EmitVTableTypeCheckedLoad(
+  if (CGF.ShouldEmitVTableTypeCheckedLoad(MethodDecl->getParent()))
+    return CGF.EmitVTableTypeCheckedLoad(
         MethodDecl->getParent(), VTable,
         VTableIndex * CGM.getContext().getTargetInfo().getPointerWidth(0) / 8);
-  } else {
-    CGF.EmitTypeMetadataCodeForVCall(MethodDecl->getParent(), VTable, Loc);
-
-    llvm::Value *VFuncPtr =
-        CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-    auto *VFuncLoad =
-        CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
-
-    // Add !invariant.load md to virtual function load to indicate that
-    // function didn't change inside vtable.
-    // It's safe to add it without -fstrict-vtable-pointers, but it would not
-    // help in devirtualization because it will only matter if we will have 2
-    // the same virtual function loads from the same vtable load, which won't
-    // happen without enabled devirtualization with -fstrict-vtable-pointers.
-    if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
-        CGM.getCodeGenOpts().StrictVTablePointers)
-      VFuncLoad->setMetadata(
-          llvm::LLVMContext::MD_invariant_load,
-          llvm::MDNode::get(CGM.getLLVMContext(),
-                            llvm::ArrayRef<llvm::Metadata *>()));
-    VFunc = VFuncLoad;
-  }
-
-  CGCallee Callee(MethodDecl, VFunc);
-  return Callee;
+
+  CGF.EmitTypeMetadataCodeForVCall(MethodDecl->getParent(), VTable, Loc);
+
+  llvm::Value *VFuncPtr =
+      CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
+  auto *VFuncLoad =
+      CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+  // Add !invariant.load md to virtual function load to indicate that
+  // function didn't change inside vtable.
+  // It's safe to add it without -fstrict-vtable-pointers, but it would not
+  // help in devirtualization because it will only matter if we will have 2
+  // the same virtual function loads from the same vtable load, which won't
+  // happen without enabled devirtualization with -fstrict-vtable-pointers.
+  if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+      CGM.getCodeGenOpts().StrictVTablePointers)
+    VFuncLoad->setMetadata(
+        llvm::LLVMContext::MD_invariant_load,
+        llvm::MDNode::get(CGM.getLLVMContext(),
+                          llvm::ArrayRef<llvm::Metadata *>()));
+  return VFuncLoad;
 }
 
 llvm::Value *ItaniumCXXABI::EmitVirtualDestructorCall(
@@ -1702,10 +1697,10 @@ llvm::Value *ItaniumCXXABI::EmitVirtualDestructorCall(
 
   const CGFunctionInfo *FInfo = &CGM.getTypes().arrangeCXXStructorDeclaration(
       Dtor, getFromDtorType(DtorType));
-  llvm::Type *Ty = CGF.CGM.getTypes().GetFunctionType(*FInfo);
+  auto *Ty =
+      cast<llvm::FunctionType>(CGF.CGM.getTypes().GetFunctionType(*FInfo));
   CGCallee Callee =
-      getVirtualFunctionPointer(CGF, GlobalDecl(Dtor, DtorType), This, Ty,
-                                CE ? CE->getLocStart() : SourceLocation());
+      CGCallee::forVirtual(CE, GlobalDecl(Dtor, DtorType), This, Ty);
 
   CGF.EmitCXXMemberOrOperatorCall(Dtor, Callee, ReturnValueSlot(),
                                   This.getPointer(), /*ImplicitParam=*/nullptr,
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 420d2843af8..046acd76ce7 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -285,9 +285,9 @@ public:
   llvm::GlobalVariable *getAddrOfVTable(const CXXRecordDecl *RD,
                                         CharUnits VPtrOffset) override;
 
-  CGCallee getVirtualFunctionPointer(CodeGenFunction &CGF, GlobalDecl GD,
-                                     Address This, llvm::Type *Ty,
-                                     SourceLocation Loc) override;
+  llvm::Value *getVirtualFunctionPointer(CodeGenFunction &CGF, GlobalDecl GD,
+                                         Address This, llvm::Type *Ty,
+                                         SourceLocation Loc) override;
 
   llvm::Value *EmitVirtualDestructorCall(CodeGenFunction &CGF,
                                          const CXXDestructorDecl *Dtor,
@@ -1827,11 +1827,11 @@ llvm::GlobalVariable *MicrosoftCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
   return VTable;
 }
 
-CGCallee MicrosoftCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
-                                                    GlobalDecl GD,
-                                                    Address This,
-                                                    llvm::Type *Ty,
-                                                    SourceLocation Loc) {
+llvm::Value *MicrosoftCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
+                                                        GlobalDecl GD,
+                                                        Address This,
+                                                        llvm::Type *Ty,
+                                                        SourceLocation Loc) {
   GD = GD.getCanonicalDecl();
   CGBuilderTy &Builder = CGF.Builder;
 
@@ -1858,22 +1858,17 @@ CGCallee MicrosoftCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
         ->ObjectWithVPtr;
   };
 
-  llvm::Value *VFunc;
-  if (CGF.ShouldEmitVTableTypeCheckedLoad(MethodDecl->getParent())) {
-    VFunc = CGF.EmitVTableTypeCheckedLoad(
+  if (CGF.ShouldEmitVTableTypeCheckedLoad(MethodDecl->getParent()))
+    return CGF.EmitVTableTypeCheckedLoad(
         getObjectWithVPtr(), VTable,
         ML.Index * CGM.getContext().getTargetInfo().getPointerWidth(0) / 8);
-  } else {
-    if (CGM.getCodeGenOpts().PrepareForLTO)
-      CGF.EmitTypeMetadataCodeForVCall(getObjectWithVPtr(), VTable, Loc);
 
-    llvm::Value *VFuncPtr =
-        Builder.CreateConstInBoundsGEP1_64(VTable, ML.Index, "vfn");
-    VFunc = Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
-  }
+  if (CGM.getCodeGenOpts().PrepareForLTO)
+    CGF.EmitTypeMetadataCodeForVCall(getObjectWithVPtr(), VTable, Loc);
 
-  CGCallee Callee(MethodDecl, VFunc);
-  return Callee;
+  llvm::Value *VFuncPtr =
+      Builder.CreateConstInBoundsGEP1_64(VTable, ML.Index, "vfn");
+  return Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
 }
 
 llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
@@ -1887,9 +1882,9 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
   GlobalDecl GD(Dtor, Dtor_Deleting);
   const CGFunctionInfo *FInfo = &CGM.getTypes().arrangeCXXStructorDeclaration(
       Dtor, StructorType::Deleting);
-  llvm::Type *Ty = CGF.CGM.getTypes().GetFunctionType(*FInfo);
-  CGCallee Callee = getVirtualFunctionPointer(
-      CGF, GD, This, Ty, CE ? CE->getLocStart() : SourceLocation());
+  auto *Ty =
+      cast<llvm::FunctionType>(CGF.CGM.getTypes().GetFunctionType(*FInfo));
+  CGCallee Callee = CGCallee::forVirtual(CE, GD, This, Ty);
 
   ASTContext &Context = getContext();
   llvm::Value *ImplicitParam = llvm::ConstantInt::get(