Generating available_externally vtables and assume loads bugfix

[rsmith]

LGTM

lib/CodeGen/ItaniumCXXABI.cpp

399–400

No newline before else.

1631

This function is now serving two purposes:

1. Can we generate a new reference to the vtable that was not implied by the source code and checked by Sema, and
2. Can we generate a speculative definition of the vtable, despite it (perhaps) not being used.

I'm OK with the current approach as a conservative fix for the issue we're seeing with visibility, but we should aim to separate out these two questions eventually. For the former, we don't need to check the vtable contents, just the visibility of the vtable symbol itself.

test/CodeGenCXX/vtable-assume-load.cpp

186–187

"and all functions aint hidden," -> "and no virtual functions are hidden,"

[majnemer]

lib/CodeGen/ItaniumCXXABI.cpp

394–406

What about the CK_DeletingDtorPointer or CK_CompleteDtorPointer cases?

[Prazek]

lib/CodeGen/ItaniumCXXABI.cpp

394–406

isUdedFunctionPointerKind check for CK_FunctionPointer, CK_CompleteDtorPointer, CK_DeletingDtorPointer

[Prazek]

Assume loads released.

include/clang/AST/VTableBuilder.h

public:
 
   CharUnits getOffsetToTop() const {
     assert(getKind() == CK_OffsetToTop && "Invalid component kind!");
 
     return getOffset();
   }
 
   const CXXRecordDecl *getRTTIDecl() const {
-    assert(getKind() == CK_RTTI && "Invalid component kind!");
+    assert(isRTTIKind() && "Invalid component kind!");
     return reinterpret_cast<CXXRecordDecl *>(getPointer());
   }
 
   const CXXMethodDecl *getFunctionDecl() const {
     assert(isFunctionPointerKind() && "Invalid component kind!");
     if (isDestructorKind())
       return getDestructorDecl();
     return reinterpret_cast<CXXMethodDecl *>(getPointer());
public:
   bool isUsedFunctionPointerKind() const {
     return isUsedFunctionPointerKind(getKind());
   }
 
   bool isFunctionPointerKind() const {
     return isFunctionPointerKind(getKind());
   }
 
+  bool isRTTIKind() const { return isRTTIKind(getKind()); }
+
 private:
   static bool isFunctionPointerKind(Kind ComponentKind) {
     return isUsedFunctionPointerKind(ComponentKind) ||
            ComponentKind == CK_UnusedFunctionPointer;
   }
   static bool isUsedFunctionPointerKind(Kind ComponentKind) {
     return ComponentKind == CK_FunctionPointer ||
            isDestructorKind(ComponentKind);
   }
   static bool isDestructorKind(Kind ComponentKind) {
     return ComponentKind == CK_CompleteDtorPointer ||
            ComponentKind == CK_DeletingDtorPointer;
   }
+  static bool isRTTIKind(Kind ComponentKind) {
+    return ComponentKind == CK_RTTI;
+  }
 
   VTableComponent(Kind ComponentKind, CharUnits Offset) {
     assert((ComponentKind == CK_VCallOffset ||
             ComponentKind == CK_VBaseOffset ||
             ComponentKind == CK_OffsetToTop) && "Invalid component kind!");
     assert(Offset.getQuantity() < (1LL << 56) && "Offset is too big!");
     assert(Offset.getQuantity() >= -(1LL << 56) && "Offset is too small!");
 
     Value = (uint64_t(Offset.getQuantity()) << 3) | ComponentKind;
   }
 
   VTableComponent(Kind ComponentKind, uintptr_t Ptr) {
-    assert((ComponentKind == CK_RTTI || isFunctionPointerKind(ComponentKind)) &&
+    assert((isRTTIKind(ComponentKind) || isFunctionPointerKind(ComponentKind)) &&
            "Invalid component kind!");
 
     assert((Ptr & 7) == 0 && "Pointer not sufficiently aligned!");
 
     Value = Ptr | ComponentKind;
   }
 
   CharUnits getOffset() const {

lib/CodeGen/CGClass.cpp

const CGFunctionInfo &Info =
       CGM.getTypes().arrangeCXXConstructorCall(Args, D, Type, ExtraArgs);
   EmitCall(Info, Callee, ReturnValueSlot(), Args, D);
 
   // Generate vtable assumptions if we're constructing a complete object
   // with a vtable.  We don't do this for base subobjects for two reasons:
   // first, it's incorrect for classes with virtual bases, and second, we're
   // about to overwrite the vptrs anyway.
   // We also have to make sure if we can refer to vtable:
-  // - If vtable is external then it's safe to use it (for available_externally
-  //   CGVTables will make sure if it can emit it).
   // - Otherwise we can refer to vtable if it's safe to speculatively emit.
-  // FIXME: If vtable is used by ctor/dtor, we are always safe to refer to it.
+  // FIXME: If vtable is used by ctor/dtor, or if vtable is external and we are
+  // sure that definition of vtable is not hidden,
+  // then we are always safe to refer to it.
   if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
       ClassDecl->isDynamicClass() && Type != Ctor_Base &&
-      (CGM.getVTables().isVTableExternal(ClassDecl) ||
-       CGM.getCXXABI().canSpeculativelyEmitVTable(ClassDecl)))
+      CGM.getCXXABI().canSpeculativelyEmitVTable(ClassDecl))
     EmitVTableAssumptionLoads(ClassDecl, This);
 }
 
 void CodeGenFunction::EmitVTableAssumptionLoad(const VPtr &Vptr, Address This) {
   llvm::Value *VTableGlobal =
       CGM.getCXXABI().getVTableAddressPoint(Vptr.Base, Vptr.VTableClass);
   if (!VTableGlobal)
     return;

lib/CodeGen/ItaniumCXXABI.cpp

for (const auto &VtableComponent : VtableLayout.vtable_components()) {
         continue;
 
       const CXXMethodDecl *Method = VtableComponent.getFunctionDecl();
       if (Method->getCanonicalDecl()->isInlined())
         return true;
     }
     return false;
   }
+
+  bool isVTableHidden(const CXXRecordDecl *RD) const {
+    const auto &VtableLayout =
+            CGM.getItaniumVTableContext().getVTableLayout(RD);
+
+    for (const auto &VtableComponent : VtableLayout.vtable_components()) {
+      if (VtableComponent.isRTTIKind()) {
+        const CXXRecordDecl *RTTIDecl = VtableComponent.getRTTIDecl();
+        if (RTTIDecl->getVisibility() == Visibility::HiddenVisibility)
+          return true;
+      } else if (VtableComponent.isUsedFunctionPointerKind()) {
+        const CXXMethodDecl *Method = VtableComponent.getFunctionDecl();
+        if (Method->getVisibility() == Visibility::HiddenVisibility &&
+            !Method->isDefined())
+          return true;
+      }
+    }
+    return false;
+  }
 };
 
 class ARMCXXABI : public ItaniumCXXABI {
 public:
   ARMCXXABI(CodeGen::CodeGenModule &CGM) :
     ItaniumCXXABI(CGM, /* UseARMMethodPtrABI = */ true,
                   /* UseARMGuardVarABI = */ true) {}
 

 }
 
 void ItaniumCXXABI::emitVirtualInheritanceTables(const CXXRecordDecl *RD) {
   CodeGenVTables &VTables = CGM.getVTables();
   llvm::GlobalVariable *VTT = VTables.GetAddrOfVTT(RD);
   VTables.EmitVTTDefinition(VTT, CGM.getVTableLinkage(RD), RD);
 }
 
 bool ItaniumCXXABI::canSpeculativelyEmitVTable(const CXXRecordDecl *RD) const {
   // We don't emit available_externally vtables if we are in -fapple-kext mode
   // because kext mode does not permit devirtualization.
   if (CGM.getLangOpts().AppleKext)
     return false;
 
-  // If we don't have any inline virtual functions,
+  // If we don't have any inline virtual functions, and if vtable is not hidden,
   // then we are safe to emit available_externally copy of vtable.
   // FIXME we can still emit a copy of the vtable if we
   // can emit definition of the inline functions.
-  return !hasAnyUsedVirtualInlineFunction(RD);
+  return !hasAnyUsedVirtualInlineFunction(RD) && !isVTableHidden(RD);
 }
 static llvm::Value *performTypeAdjustment(CodeGenFunction &CGF,
                                           Address InitialPtr,
                                           int64_t NonVirtualAdjustment,
                                           int64_t VirtualAdjustment,
                                           bool IsReturnAdjustment) {
   if (!NonVirtualAdjustment && !VirtualAdjustment)
     return InitialPtr.getPointer();

test/CodeGenCXX/vtable-assume-load.cpp

 // RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -emit-llvm -o %t.ll -O1 -disable-llvm-optzns -fms-extensions
 // RUN: %clang_cc1 %s -triple i686-pc-win32 -emit-llvm -o %t.ms.ll -O1 -disable-llvm-optzns -fms-extensions
 
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK3 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK4 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK-MS --input-file=%t.ms.ll %s
 // RUN: FileCheck --check-prefix=CHECK6 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK7 --input-file=%t.ll %s
 // RUN: FileCheck --check-prefix=CHECK8 --input-file=%t.ll %s
+// RUN: FileCheck --check-prefix=CHECK9 --input-file=%t.ll %s
 namespace test1 {
 
 struct A {
   A();
   virtual void foo();
 };
 
 struct B : A {

 void test() {
   S s;
   g(s);
 }
 
 } // testMS
 
 namespace test6 {
-// CHECK6: @_ZTVN5test61AE = external
 struct A {
   A();
   virtual void foo();
   virtual ~A() {}
 };
 struct B : A {
   B();
 };
-// Because A's vtable is external, it's safe to generate assumption loads.
+// FIXME: Because A's vtable is external, and no virtual functions are hidden,
+// it's safe to generate assumption loads.
 // CHECK6-LABEL: define void @_ZN5test61gEv()
 // CHECK6: call void @_ZN5test61AC1Ev(
-// CHECK6: call void @llvm.assume(
+// CHECK6-NOT: call void @llvm.assume(
 
 // We can't emit assumption loads for B, because if we would refer to vtable
 // it would refer to functions that will not be able to find (like implicit
 // inline destructor).
 
 // CHECK6-LABEL:   call void @_ZN5test61BC1Ev(
 // CHECK6-NOT: call void @llvm.assume(
 // CHECK6-LABEL: }

 // CHECK8-DAG: @_ZTVN5test81CE = linkonce_odr unnamed_addr constant
 struct C : A {
   C();
   void bar();
   void foo() {}
 };
 inline void C::bar() {}
 
-// CHECK8-DAG: @_ZTVN5test81DE = external unnamed_addr constant
 struct D : A {
   D();
   void foo();
   void inline bar();
 };
 void D::bar() {}
 
 // CHECK8-DAG: @_ZTVN5test81EE = linkonce_odr unnamed_addr constant

 // CHECK8-LABEL: define void @_ZN5test81cEv()
 // CHECK8-NOT: call void @llvm.assume(
 // CHECK8-LABEL: }
 void c() {
   C c;
   c.bar();
 }
 
+// FIXME: We could generate assumption loads here.
 // CHECK8-LABEL: define void @_ZN5test81dEv()
-// CHECK8: call void @llvm.assume(
+// CHECK8-NOT: call void @llvm.assume(
 // CHECK8-LABEL: }
 void d() {
   D d;
   d.bar();
 }
 
 // CHECK8-LABEL: define void @_ZN5test81eEv()
 // CHECK8: call void @llvm.assume(
 // CHECK8-LABEL: }
 void e() {
   E e;
   e.bar();
 }
 }
+
+namespace test9 {
+
+struct S {
+  __attribute__((visibility("hidden"))) S();
+  virtual void doStuff();
+};
+
+// CHECK9-LABEL: define void @_ZN5test94testEv()
+// CHECK9-NOT: @llvm.assume(
+// CHECK9: }
+void test() {
+  S *s = new S();
+  s->doStuff();
+  delete s;
+}
+}
+

test/CodeGenCXX/vtable-available-externally.cpp

 // RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -emit-llvm -o %t
 // RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -O2 -disable-llvm-optzns -emit-llvm -o %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST1 %s < %t
 // RUN: FileCheck --check-prefix=CHECK-TEST2 %s < %t
 // RUN: FileCheck --check-prefix=CHECK-TEST5 %s < %t
 // RUN: FileCheck --check-prefix=CHECK-TEST8 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST9 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST10 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST11 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST12 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST13 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST14 %s < %t.opt
 // RUN: FileCheck --check-prefix=CHECK-TEST15 %s < %t.opt
+// RUN: FileCheck --check-prefix=CHECK-TEST16 %s < %t.opt
 
 #include <typeinfo>
 
 // CHECK-TEST1: @_ZTVN5Test11AE = external unnamed_addr constant
 namespace Test1 {
 
 struct A {
   A();

 };
 
 void test() {
   D * d = new D;
   d->f();
 }
 }
 
+namespace Test16 {
+// S has virtual method that is hidden, because of it we can't
+// generate available_externally vtable for it.
+// CHECK-TEST16-DAG: @_ZTVN6Test161SE = external unnamed_addr constant
+// CHECK-TEST16-DAG: @_ZTVN6Test162S2E = available_externally
+
+struct S {
+  __attribute__((visibility("hidden"))) virtual void doStuff();
+};
+
+struct S2 {
+  virtual void doStuff();
+  __attribute__((visibility("hidden"))) void unused();
+
+};
+
+void test() {
+  S *s = new S;
+  s->doStuff();
+
+  S2 *s2 = new S2;
+  s2->doStuff();
+}
+}
+