[PS4] handle dllimport/export w.r.t vtables/rtti

[bd1976llvm]

Summary

The existing Windows Itanium patches for dllimport/export behaviour w.r.t vtables/rtti can't be adopted for PS4 due to backwards compatibility reasons (see comments on https://reviews.llvm.org/D90299).

This review is for adding our PS4 scheme for this to Clang.

[compnerd]

clang/include/clang/Basic/TargetInfo.h

1109

Please reflow the comment (or clang-format)

clang/lib/CodeGen/ItaniumCXXABI.cpp

1823

Can this not be const auto *D : RD->noload_decls()?

1824–1830

I think that the additional conditional and early break are a bit easier to read.

1902

I'm having a hard time following this comment.

I don't think that "Within the context of the Itanium C++ ABI, we want to mimic this MS behavior." adds any value.

What exactly does "defined in this TU/externally" mean? TU or defined externally means everywhere, no?

What is the "MS constraint"?

3156

Please reflow the text (or clang-format)

3799

I'm confused - does PS4 use a custom scheme or the MS ABI rules?

[bd1976llvm]

clang/lib/CodeGen/ItaniumCXXABI.cpp

1902

I will reword.

What is the "MS constraint"?

This is the constraint from the first sentence: "when dllimport/exporting only certain members of a class (rather than the non-inline virtual methods must be dllimported/exported, otherwise a link error will result". This constraint is documented here: https://docs.microsoft.com/en-us/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes?view=msvc-160#_pluslang_using_dllimport_and_dllexport_in_c2b2bselectivememberimportexport as "If you have a class in which you are using selective member import/export with virtual functions, the functions must be in the exportable interface or defined inline (visible to the client).".

What exactly does "defined in this TU/externally" mean? TU or defined externally means everywhere, no?

Sorry badly phrased, try: "For classes using selective member import/export: We dllimport the vtable if it is defined externally and all the non-inline virtual methods are marked dllimport. We dllexport the vtable if it is defined in this TU and all the non-inline virtual methods are marked dllexport. Otherwise the vtable is not imported or exported. With this scheme links succeed in circumstances where they would succeed for Microsoft toolchains and fail with a linker error (missing vtable for struct) in circumstances where MS toolchains would give a linker error.

Hope that makes it clearer. There is a wordier comment in the attached test explaining this.

3799

Sorry. Confusingly written :( The "custom scheme" I'm referring to is the patches in this review. The goal is to match the MS toolchain behaviour for dllimport/export on classes with vtables. This comment should probably just state: " We export the typeinfo in the same circumstances as the vtable is exported.".

[bd1976llvm]

Thanks for the suggestions. I have reworded the comments so that they are hopefully much clearer.

[bd1976llvm]

I have added this patch to https://reviews.llvm.org/D88124 along with some end to end tests to show the scei vendor windows-itanium behaviour. From the 'wi-test/test/dll_vtable_missing/test.py' test you can see that the link error given by the scei vendor variant is more consistent than the standard windows-itanium. Both variants should be equivalent in terms of runtime behaviour.

[bd1976llvm]

ping!

[bd1976llvm]

ping.

[bd1976llvm]

@compnerd, @smeenai do you have any further comments?

[probinson]

FTR, this looks okay from the PS4 owner's perspective, although IIRC @wristow did our original local patches.

@compnerd it looks like @bd1976bris has addressed all the comments; is there anything blocking an LGTM here?

[smeenai]

In D93203#2647578, @bd1976llvm wrote:

@compnerd, @smeenai do you have any further comments?

Sorry for the lack of review on my part here; I haven't had any time to look at this in detail.

[wristow]

LGTM!

clang/include/clang/Basic/TargetInfo.h

public:
 
   /// Does this target aim for semantic compatibility with
   /// Microsoft C++ code using dllimport/export attributes?
   virtual bool shouldDLLImportComdatSymbols() const {
     return getTriple().isWindowsMSVCEnvironment() ||
            getTriple().isWindowsItaniumEnvironment() || getTriple().isPS4CPU();
   }
 
+  // Does this target have PS4 specific
+  // dllimport/export handling?
+  virtual bool hasPS4DLLImportExport() const {
+    return getTriple().isPS4CPU() ||
+           // Windows Itanium support allows for testing the SCEI flavour
+           // of dllimport/export handling on a Windows system.
+           (getTriple().isWindowsItaniumEnvironment() && getTriple().getVendor() == llvm::Triple::SCEI);
+  }
+
   /// An optional hook that targets can implement to perform semantic
   /// checking on attribute((section("foo"))) specifiers.
   ///
   /// In this case, "foo" is passed in to be checked.  If the section
   /// specifier is invalid, the backend should return a non-empty string
   /// that indicates the problem.
   ///
   /// This hook is a simple quality of implementation feature to catch errors

clang/lib/AST/RecordLayoutBuilder.cpp

if (Context.getLangOpts().CUDA) {
         if (!MD->hasAttr<CUDAHostAttr>() && MD->hasAttr<CUDADeviceAttr>())
           continue;
       }
     }
 
     // If the key function is dllimport but the class isn't, then the class has
     // no key function. The DLL that exports the key function won't export the
     // vtable in this case.
-    if (MD->hasAttr<DLLImportAttr>() && !RD->hasAttr<DLLImportAttr>())
+    if (MD->hasAttr<DLLImportAttr>() && !RD->hasAttr<DLLImportAttr>() &&
+        !Context.getTargetInfo().hasPS4DLLImportExport())
       return nullptr;
 
     // We found it.
     return MD;
   }
 
   return nullptr;
 }

clang/lib/CodeGen/ItaniumCXXABI.cpp

llvm::Value *Indices[] = {
     llvm::ConstantInt::get(CGM.Int32Ty, AddressPoint.AddressPointIndex),
   };
 
   return llvm::ConstantExpr::getGetElementPtr(VTable->getValueType(), VTable,
                                               Indices, /*InBounds=*/true,
                                               /*InRangeIndex=*/1);
 }
 
+// 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 (auto *D : RD->noload_decls()) {
+    FunctionDecl *FD;
+    if ((FD = dyn_cast<FunctionDecl>(D)) && FD->isVirtualAsWritten() &&
+        !FD->isInlineSpecified() && !FD->doesThisDeclarationHaveABody()) {
+      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) {
   assert((Base.getBase()->getNumVBases() || NearestVBase != nullptr) &&
          NeedsVTTParameter(CGF.CurGD) && "This class doesn't have VTT");
 
   // Get the secondary vpointer index.
   uint64_t VirtualPointerIndex =
unsigned PAlign = CGM.getItaniumVTableContext().isRelativeLayout()
                         ? 32
                         : CGM.getTarget().getPointerAlign(0);
 
   VTable = CGM.CreateOrReplaceCXXRuntimeVariable(
       Name, VTableType, llvm::GlobalValue::ExternalLinkage,
       getContext().toCharUnitsFromBits(PAlign).getQuantity());
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
 
+  // The MS C++ ABI has the constraint that, when dllimport/exporting
+  // only certain members of a class (rather than the whole class), all the
+  // non-inline virtual methods must be dllimported/exported otherwise a
+  // link error will result. Within the context of the Itanium C++ ABI, we want
+  // to mimic this MS behavior. For such clases we dllimport/export the vtable
+  // if it is defined in this TU/externally and the MS constraint is met; but
+  // we don't do so if the MS contraint is not met. This results in a link error
+  // (missing vtable for struct) if the MS constraint is violated.
+  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);
 
   return VTable;
 }
 
 CGCallee ItaniumCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
                                                   GlobalDecl GD,
                                                   Address This,
if (!GV) {
     // RTTI, check if emitting vtables opportunistically need any adjustment.
 
     GV = new llvm::GlobalVariable(CGM.getModule(), CGM.Int8PtrTy,
                                   /*isConstant=*/true,
                                   llvm::GlobalValue::ExternalLinkage, nullptr,
                                   Name);
     const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
     CGM.setGVProperties(GV, RD);
+    // Import the typeinfo symbol when all
+    // non-inline virtual methods are imported.
+    if (CGM.getTarget().hasPS4DLLImportExport()) {
+      if (RD && CXXRecordAllNonInlineVirtualsHaveAttr<DLLImportAttr>(RD)) {
+        GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
+        CGM.setDSOLocal(GV);
+      }
+    }
   }
 
   return llvm::ConstantExpr::getBitCast(GV, CGM.Int8PtrTy);
 }
 
 /// TypeInfoIsInStandardLibrary - Given a builtin type, returns whether the type
 /// info for that type is defined in the standard library.
 static bool TypeInfoIsInStandardLibrary(const BuiltinType *Ty) {
if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
     // N.B. We must always emit the RTTI data ourselves if there exists a key
     // function.
     bool IsDLLImport = RD->hasAttr<DLLImportAttr>();
 
     // Don't import the RTTI but emit it locally.
     if (CGM.getTriple().isWindowsGNUEnvironment())
       return false;
 
-    if (CGM.getVTables().isVTableExternal(RD))
+    if (CGM.getVTables().isVTableExternal(RD)) {
+      if (CGM.getTarget().hasPS4DLLImportExport())
+        return true;
+
       return IsDLLImport && !CGM.getTriple().isWindowsItaniumEnvironment()
                  ? false
                  : true;
-
+    }
     if (IsDLLImport)
       return true;
   }
 
   return false;
 }
 
 /// IsIncompleteClassType - Returns whether the given record type is incomplete.
#include "clang/AST/TypeNodes.inc"
   llvm::raw_svector_ostream Out(Name);
   CGM.getCXXABI().getMangleContext().mangleCXXRTTI(Ty, Out);
   llvm::Module &M = CGM.getModule();
   llvm::GlobalVariable *OldGV = M.getNamedGlobal(Name);
   llvm::GlobalVariable *GV =
       new llvm::GlobalVariable(M, Init->getType(),
                                /*isConstant=*/true, Linkage, Init, Name);
 
+  // PS4 has a custom scheme for dllexporting vtables so that
+  // using dllimport/export on classes matches the MS ABI rules. We
+  // We must export the typeinfo in the same circumstances as the
+  // vtable is exported.
+  auto GVDLLStorageClass = DLLStorageClass;
+  if (CGM.getTarget().hasPS4DLLImportExport()) {
+    if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
+      const CXXRecordDecl *RD = cast<CXXRecordDecl>(RecordTy->getDecl());
+      if (RD->hasAttr<DLLExportAttr>() ||
+          CXXRecordAllNonInlineVirtualsHaveAttr<DLLExportAttr>(RD)) {
+        GVDLLStorageClass = llvm::GlobalVariable::DLLExportStorageClass;
+      }
+    }
+  }
+
   // If there's already an old global variable, replace it with the new one.
   if (OldGV) {
     GV->takeName(OldGV);
     llvm::Constant *NewPtr =
       llvm::ConstantExpr::getBitCast(GV, OldGV->getType());
     OldGV->replaceAllUsesWith(NewPtr);
     OldGV->eraseFromParent();
   }
#include "clang/AST/TypeNodes.inc"
 
   TypeName->setVisibility(Visibility);
   CGM.setDSOLocal(TypeName);
 
   GV->setVisibility(Visibility);
   CGM.setDSOLocal(GV);
 
   TypeName->setDLLStorageClass(DLLStorageClass);
-  GV->setDLLStorageClass(DLLStorageClass);
+  GV->setDLLStorageClass(CGM.getTarget().hasPS4DLLImportExport()
+                             ? GVDLLStorageClass
+                             : DLLStorageClass);
 
   TypeName->setPartition(CGM.getCodeGenOpts().SymbolPartition);
   GV->setPartition(CGM.getCodeGenOpts().SymbolPartition);
 
   return llvm::ConstantExpr::getBitCast(GV, CGM.Int8PtrTy);
 }
 
 /// BuildObjCObjectTypeInfo - Build the appropriate kind of type_info

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

+// For a class that has a vtable (and hence, also has a typeinfo symbol for
+// RTTI), if a user marks either:
+//
+//  (a) the entire class as dllexport (dllimport), or
+//  (b) all non-inline virtual methods 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.
+//
+// Conversely to point (b), if some (but not all) of the non-inline virtual
+// methods of a class are marked as dllexport (dllimport), then the vtable and
+// typeinfo symbols must not be exported (imported).  This will result in a
+// link-time failure when linking the importing module.  This link-time failure
+// is the desired behavior, because the Microsoft toolchain also gets a
+// link-time failure in these cases (and since __declspec(dllexport)
+// (__declspec(dllimport)) is a Microsoft extension, our intention is to mimic
+// that Microsoft behavior).
+//
+// Side note: It is within the bodies of constructors (and in some cases,
+// destructors) that the vtable is explicitly referenced.  In case (a) above,
+// where the entire class is exported (imported), then all constructors (among
+// other things) are exported (imported).  So for that situation, an importing
+// module for a well-formed program will not actually reference the vtable,
+// since constructor calls will all be to functions external to that module
+// (and imported into it, from the exporting module).  I.e., all vtable
+// references will be in that module where the constructor and destructor
+// bodies are, therefore, there will not be a need to import the vtable in
+// that case.
+//
+// This test contains 6 test classes:
+//   2 for point (a),
+//   2 for point (b),
+//   and 2 negative tests for the converse of point (b).
+//
+// The two tests for each of these points are one for importing, and one for
+// exporting.
+
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-unknown-windows-itanium -emit-llvm -o - %s | FileCheck %s -check-prefix=WI
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-scei-windows-itanium -emit-llvm -o - %s | FileCheck %s --check-prefixes=PS4,SCEI_WI
+// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-scei-ps4 -emit-llvm -o - %s | FileCheck %s --check-prefixes=PS4,SCEI_PS4
+
+#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
+// (as described in the "Side note", above).
+//
+// PS4-DAG: @_ZTI10FullImport = {{.*}}dllimport
+// WI-DAG: @_ZTI10FullImport = external dllimport constant i8*
+struct __declspec(dllimport) FullImport
+{
+  virtual void getId() {}
+  virtual void Bump();
+  virtual void Decrement();
+};
+
+// 'FullImport::Bump()' is the key function, so the vtable and typeinfo symbol
+// of 'FullImport' will be defined in the TU that contains the definition of
+// 'Bump()' (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,
+//
+// PS4-DAG: @_ZTV10FullExport ={{.*}}dllexport
+// WI-DAG: @_ZTV10FullExport ={{.*}}dllexport
+// PS4-DAG: @_ZTI10FullExport ={{.*}}dllexport
+// WI-DAG: @_ZTI10FullExport = dso_local dllexport constant {
+struct __declspec(dllexport) FullExport // Easy case: Entire class is exported.
+{
+  virtual void getId() {}
+  virtual void Bump();
+  virtual void Decrement();
+};
+
+// 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::Bump()
+{
+  typeid(FullExport).name();
+}
+
+///////////////////////////////////////////////////////////////////
+
+// Case (b) -- Import Aspect
+// The class as a whole is not imported, but all non-inline virtual methods of
+// the class are, so the vtable and typeinfo symbol must be imported.
+//
+// PS4-DAG: @_ZTV9FooImport ={{.*}}dllimport
+// WI-DAG:  @_ZTV9FooImport = linkonce_odr dso_local unnamed_addr constant {
+// PS4-DAG: @_ZTI9FooImport ={{.*}}dllimport
+// WI-DAG:  @_ZTI9FooImport = linkonce_odr dso_local constant {
+
+
+struct FooImport
+{
+  virtual void getId() const {}
+  __declspec(dllimport) virtual void Bump();
+  __declspec(dllimport) virtual void Decrement();
+};
+
+// 'FooImport::Bump()' is the key function, so the vtable and typeinfo symbol
+// of 'FooImport' will be defined in the TU that contains the definition of
+// 'Bump()' (and they must be exported from there).  Here, we will reference
+// the vtable and typeinfo symbol, so we must also import them.
+void importTest()
+{
+  typeid(FooImport).name();
+}
+
+///////////////////////////////////////////////////////////////////
+
+// Case (b) -- Export Aspect
+// The class as a whole is not exported, but all non-inline virtual methods of
+// the class are, so the vtable and typeinfo symbol must be exported.
+//
+// PS4-DAG: @_ZTV9FooExport ={{.*}}dllexport
+// WI-DAG:  @_ZTV9FooExport = dso_local unnamed_addr constant {
+// PS4-DAG: @_ZTI9FooExport ={{.*}}dllexport
+// WI-DAG:  @_ZTI9FooExport = dso_local constant {
+struct FooExport
+{
+  virtual void getId() const {}
+  __declspec(dllexport) virtual void Bump();
+  __declspec(dllexport) virtual void Decrement();
+};
+
+// This is the key function of the class 'FooExport', so the vtable and
+// typeinfo symbol of 'FooExport' will be defined in this TU, and so they must
+// be exported from this TU.
+void FooExport::Bump()
+{
+  FooImport f;
+  typeid(FooExport).name();
+}
+
+///////////////////////////////////////////////////////////////////
+
+// The tests below verify that the associated vtable and typeinfo symbols are
+// not imported/exported.  These are the converse of case (b).
+//
+// Note that ultimately, if the module doing the importing calls a constructor
+// of the class with the vtable, or makes a reference to the typeinfo symbol of
+// the class, then this will result in an unresolved reference (to the vtable
+// or typeinfo symbol) when linking the importing module, and thus a link-time
+// failure.
+//
+// Note that with the Microsoft toolchain there will also be a link-time
+// failure when linking the module doing the importing.  With the Microsoft
+// toolchain, it will be an unresolved reference to the method 'Decrement()'
+// of the approriate class, rather than to the vtable or typeinfo symbol of
+// the class, because Microsoft defines the vtable and typeinfo symbol (weakly)
+// everywhere they are used.
+
+// Converse of case (b) -- Import Aspect
+// The class as a whole is not imported, and not all non-inline virtual methods
+// are imported, so the vtable and typeinfo symbol are not to be imported.
+//
+// CHECK-PS4: @_ZTV11FooNoImport = external dso_local unnamed_addr constant { 
+// CHECK-WI:  @_ZTV11FooNoImport = linkonce_odr dso_local unnamed_addr constant {
+// CHECK-PS4: @_ZTI11FooNoImport = external dso_local constant i8*{{$}}
+// CHECK-WI:  @_ZTI11FooNoImport = linkonce_odr dso_local constant {
+struct FooNoImport
+{
+  virtual void getId() const {}
+  __declspec(dllimport) virtual void Bump();
+  virtual void Decrement();     // Not imported.
+  int mCounter;
+};
+
+void importNegativeTest()
+{
+  FooNoImport f;
+  typeid(FooNoImport).name();
+}
+
+///////////////////////////////////////////////////////////////////
+
+// Converse of case (b) -- Export Aspect
+// The class as a whole is not exported, and not all non-inline virtual methods
+// are exported, so the vtable and typeinfo symbol are not to be exported.
+//
+// SCEI_PS4-DAG: @_ZTV11FooNoImport = external unnamed_addr constant { 
+// SCEI_WI-DAG: @_ZTV11FooNoExport = dso_local unnamed_addr constant {
+
+// WI-DAG:       @_ZTV11FooNoExport = dso_local unnamed_addr constant {
+// SCEI_PS4-DAG: @_ZTI11FooNoExport = constant {
+// SCEI_WI-DAG:  @_ZTI11FooNoExport = dso_local constant {
+// WI-DAG:       @_ZTI11FooNoExport = dso_local constant {
+struct FooNoExport
+{
+  virtual void getId() const {}
+  __declspec(dllexport) virtual void Bump();
+  virtual void Decrement(); // Not exported.
+  int mCounter;
+};
+
+void FooNoExport::Bump()
+{
+  typeid(FooNoExport).name();
+}