[clang] Fix PS "selective" DLL import/export of vtable & typeinfo (#92579)

Prior to this patch, for "selective" DLL import/export, the vtable &
typeinfo would be imported/exported on the condition that all non-inline
virtual methods are imported/exported. This condition was based upon MS
guidelines related to "selective" DLL import/export.

However, in reality, this condition is too rigid and can result in
undefined vtable & typeinfo symbols for code that builds fine with MSVC.
Therefore, relax this condition to be if any non-inline method is
imported/exported.

Author: nga888

clang/lib/CodeGen/ItaniumCXXABI.cpp

@@ -1793,6 +1793,37 @@ void ItaniumCXXABI::EmitDestructorCall(CodeGenFunction &CGF,
                             ThisTy, VTT, VTTTy, nullptr);
 }
 
+// Check if any non-inline method has the specified attribute.
+template <typename T>
+static bool CXXRecordNonInlineHasAttr(const CXXRecordDecl *RD) {
+  for (const auto *D : RD->noload_decls()) {
+    if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+      if (FD->isInlined() || FD->doesThisDeclarationHaveABody() ||
+          FD->isPureVirtual())
+        continue;
+      if (D->hasAttr<T>())
+        return true;
+    }
+  }
+
+  return false;
+}
+
+static void setVTableSelectiveDLLImportExport(CodeGenModule &CGM,
+                                              llvm::GlobalVariable *VTable,
+                                              const CXXRecordDecl *RD) {
+  if (VTable->getDLLStorageClass() !=
+          llvm::GlobalVariable::DefaultStorageClass ||
+      RD->hasAttr<DLLImportAttr>() || RD->hasAttr<DLLExportAttr>())
+    return;
+
+  if (CGM.getVTables().isVTableExternal(RD)) {
+    if (CXXRecordNonInlineHasAttr<DLLImportAttr>(RD))
+      VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
+  } else if (CXXRecordNonInlineHasAttr<DLLExportAttr>(RD))
+    VTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
+}
+
 void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
                                           const CXXRecordDecl *RD) {
   llvm::GlobalVariable *VTable = getAddrOfVTable(RD, CharUnits());
@@ -1818,6 +1849,9 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
   if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
     VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
+  if (CGM.getTarget().hasPS4DLLImportExport())
+    setVTableSelectiveDLLImportExport(CGM, VTable, RD);
+
   // Set the right visibility.
   CGM.setGVProperties(VTable, RD);
 
@@ -1905,29 +1939,6 @@ ItaniumCXXABI::getVTableAddressPoint(BaseSubobject Base,
       VTable->getValueType(), VTable, Indices, /*InBounds=*/true, InRange);
 }
 
-// Check whether all the non-inline virtual methods for the class have the
-// specified attribute.
-template <typename T>
-static bool CXXRecordAllNonInlineVirtualsHaveAttr(const CXXRecordDecl *RD) {
-  bool FoundNonInlineVirtualMethodWithAttr = false;
-  for (const auto *D : RD->noload_decls()) {
-    if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
-      if (!FD->isVirtualAsWritten() || FD->isInlineSpecified() ||
-          FD->doesThisDeclarationHaveABody())
-        continue;
-      if (!D->hasAttr<T>())
-        return false;
-      FoundNonInlineVirtualMethodWithAttr = true;
-    }
-  }
-
-  // We didn't find any non-inline virtual methods missing the attribute.  We
-  // will return true when we found at least one non-inline virtual with the
-  // attribute.  (This lets our caller know that the attribute needs to be
-  // propagated up to the vtable.)
-  return FoundNonInlineVirtualMethodWithAttr;
-}
-
 llvm::Value *ItaniumCXXABI::getVTableAddressPointInStructorWithVTT(
     CodeGenFunction &CGF, const CXXRecordDecl *VTableClass, BaseSubobject Base,
     const CXXRecordDecl *NearestVBase) {
@@ -1981,26 +1992,10 @@ llvm::GlobalVariable *ItaniumCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
       getContext().toCharUnitsFromBits(PAlign).getAsAlign());
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
 
-  // In MS C++ if you have a class with virtual functions in which you are using
-  // selective member import/export, then all virtual functions must be exported
-  // unless they are inline, otherwise a link error will result. To match this
-  // behavior, for such classes, we dllimport the vtable if it is defined
-  // externally and all the non-inline virtual methods are marked dllimport, and
-  // we dllexport the vtable if it is defined in this TU and all the non-inline
-  // virtual methods are marked dllexport.
-  if (CGM.getTarget().hasPS4DLLImportExport()) {
-    if ((!RD->hasAttr<DLLImportAttr>()) && (!RD->hasAttr<DLLExportAttr>())) {
-      if (CGM.getVTables().isVTableExternal(RD)) {
-        if (CXXRecordAllNonInlineVirtualsHaveAttr<DLLImportAttr>(RD))
-          VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
-      } else {
-        if (CXXRecordAllNonInlineVirtualsHaveAttr<DLLExportAttr>(RD))
-          VTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
-      }
-    }
-  }
-  CGM.setGVProperties(VTable, RD);
+  if (CGM.getTarget().hasPS4DLLImportExport())
+    setVTableSelectiveDLLImportExport(CGM, VTable, RD);
 
+  CGM.setGVProperties(VTable, RD);
   return VTable;
 }
 
@@ -3285,7 +3280,7 @@ ItaniumRTTIBuilder::GetAddrOfExternalRTTIDescriptor(QualType Ty) {
     // Import the typeinfo symbol when all non-inline virtual methods are
     // imported.
     if (CGM.getTarget().hasPS4DLLImportExport()) {
-      if (RD && CXXRecordAllNonInlineVirtualsHaveAttr<DLLImportAttr>(RD)) {
+      if (RD && CXXRecordNonInlineHasAttr<DLLImportAttr>(RD)) {
         GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
         CGM.setDSOLocal(GV);
       }
@@ -3938,13 +3933,13 @@ llvm::Constant *ItaniumRTTIBuilder::BuildTypeInfo(
 
   // Export the typeinfo in the same circumstances as the vtable is exported.
   auto GVDLLStorageClass = DLLStorageClass;
-  if (CGM.getTarget().hasPS4DLLImportExport()) {
+  if (CGM.getTarget().hasPS4DLLImportExport() &&
+      GVDLLStorageClass != llvm::GlobalVariable::DLLExportStorageClass) {
     if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
       const CXXRecordDecl *RD = cast<CXXRecordDecl>(RecordTy->getDecl());
       if (RD->hasAttr<DLLExportAttr>() ||
-          CXXRecordAllNonInlineVirtualsHaveAttr<DLLExportAttr>(RD)) {
+          CXXRecordNonInlineHasAttr<DLLExportAttr>(RD))
         GVDLLStorageClass = llvm::GlobalVariable::DLLExportStorageClass;
-      }
     }
   }
 
@@ -3984,9 +3979,7 @@ llvm::Constant *ItaniumRTTIBuilder::BuildTypeInfo(
   CGM.setDSOLocal(GV);
 
   TypeName->setDLLStorageClass(DLLStorageClass);
-  GV->setDLLStorageClass(CGM.getTarget().hasPS4DLLImportExport()
-                             ? GVDLLStorageClass
-                             : DLLStorageClass);
+  GV->setDLLStorageClass(GVDLLStorageClass);
 
   TypeName->setPartition(CGM.getCodeGenOpts().SymbolPartition);
   GV->setPartition(CGM.getCodeGenOpts().SymbolPartition);

clang/test/CodeGenCXX/ps-dllstorage-vtable-rtti.cpp

@@ -0,0 +1,114 @@
+/// For a class that has a vtable and typeinfo symbol for RTTI, if a user marks
+/// either:
+///
+///   (a) The entire class as dllexport (dllimport)
+///   (b) Any non-inline method of the class as dllexport (dllimport)
+///
+/// then Clang must export the vtable and typeinfo symbol from the TU where they
+/// are defined (the TU containing the definition of the Itanium C++ ABI "key
+/// function") and must import them in other modules where they are referenced.
+
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-unknown-windows-itanium -emit-llvm -o - %s -fhalf-no-semantic-interposition \
+// RUN:   | FileCheck %s -check-prefix=WI
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-scei-windows-itanium    -emit-llvm -o - %s -fhalf-no-semantic-interposition \
+// RUN:   | FileCheck %s --check-prefixes=PS
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-scei-ps4 -emit-llvm -o - %s -fhalf-no-semantic-interposition \
+// RUN:   | FileCheck %s --check-prefixes=PS
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-sie-ps5  -emit-llvm -o - %s -fhalf-no-semantic-interposition \
+// RUN:   | FileCheck %s --check-prefixes=PS
+
+#include <typeinfo>
+
+/// Case (a) -- Import Aspect
+/// The entire class is imported. The typeinfo symbol must also be imported, but
+/// the vtable will not be referenced, and so does not need to be imported.
+
+// PS-DAG: @_ZTI10FullImport = {{.*}}dllimport
+// WI-DAG: @_ZTI10FullImport = external dllimport constant ptr
+struct __declspec(dllimport) FullImport {
+  virtual void inlineFunc() const {}
+  virtual void key();
+  virtual void func();
+};
+
+/// 'FullImport::key()' is the key function, so the vtable and typeinfo symbol
+/// of 'FullImport' will be defined in the TU that contains the definition of
+/// 'key()' (and they must be exported from there).
+void FullImportTest() { typeid(FullImport).name(); }
+
+/// Case (a) -- Export Aspect
+/// The entire class is exported. The vtable and typeinfo symbols must also be
+/// exported.
+
+// PS-DAG: @_ZTV10FullExport = {{.*}}dllexport
+// WI-DAG: @_ZTV10FullExport = {{.*}}dllexport
+// PS-DAG: @_ZTI10FullExport = {{.*}}dllexport
+// WI-DAG: @_ZTI10FullExport = dso_local dllexport constant {
+struct __declspec(dllexport) FullExport {
+  virtual void inlineFunc() const {}
+  virtual void key();
+  virtual void func();
+};
+
+/// This is the key function of the class 'FullExport', so the vtable and
+/// typeinfo symbols of 'FullExport' will be defined in this TU, and so they
+/// must be exported from this TU.
+void FullExport::key() { typeid(FullExport).name(); }
+
+/// Case (b) -- Import Aspect
+/// The class as a whole is not imported, but a non-inline method of the class
+/// is, so the vtable and typeinfo symbol must be imported.
+
+// PS-DAG: @_ZTV10PartImport = {{.*}}dllimport
+// WI-DAG: @_ZTV10PartImport = external dso_local unnamed_addr constant {
+// PS-DAG: @_ZTI10PartImport = {{.*}}dllimport
+// WI-DAG: @_ZTI10PartImport = external dso_local constant ptr
+struct PartImport {
+  virtual void inlineFunc() const {}
+  virtual void key();
+  __declspec(dllimport) virtual void func();
+};
+
+/// 'PartImport::key()' is the key function, so the vtable and typeinfo symbol
+/// of 'PartImport' will be defined in the TU that contains the definition of
+/// 'key()' (and they must be exported from there). Here, we will reference the
+/// vtable and typeinfo symbol, so we must also import them.
+void PartImportTest() {
+  PartImport f;
+  typeid(PartImport).name();
+}
+
+/// Case (b) -- Export Aspect
+/// The class as a whole is not exported, but a non-inline method of the class
+/// is, so the vtable and typeinfo symbol must be exported.
+
+// PS-DAG: @_ZTV10PartExport = {{.*}}dllexport
+// WI-DAG: @_ZTV10PartExport = dso_local unnamed_addr constant {
+// PS-DAG: @_ZTI10PartExport = {{.*}}dllexport
+// WI-DAG: @_ZTI10PartExport = dso_local constant {
+struct PartExport {
+  virtual void inlineFunc() const {}
+  virtual void key();
+  __declspec(dllexport) virtual void func();
+};
+
+/// This is the key function of the class 'PartExport', so the vtable and
+/// typeinfo symbol of 'PartExport' will be defined in this TU, and so they must
+/// be exported from this TU.
+void PartExport::key() { typeid(PartExport).name(); }
+
+/// Case (b) -- Export Aspect
+/// The class as a whole is not exported, but the constructor of the class
+/// is, so the vtable and typeinfo symbol must be exported.
+
+// PS-DAG: @_ZTV10ConsExport = {{.*}}dllexport
+// WI-DAG: @_ZTV10ConsExport = dso_local unnamed_addr constant {
+// PS-DAG: @_ZTI10ConsExport = {{.*}}dllexport
+// WI-DAG: @_ZTI10ConsExport = dso_local constant {
+struct ConsExport {
+  __declspec(dllexport) ConsExport();
+  virtual void key();
+};
+
+ConsExport::ConsExport() {}
+void ConsExport::key() { typeid(ConsExport).name(); }