Author: Andrew Ng Date: 2024-05-24T14:04:22+01:00 New Revision: 0ad4c8075985a0b82c01b28750a49e9e46a8c220
URL: https://github.com/llvm/llvm-project/commit/0ad4c8075985a0b82c01b28750a49e9e46a8c220 DIFF: https://github.com/llvm/llvm-project/commit/0ad4c8075985a0b82c01b28750a49e9e46a8c220.diff LOG: [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. Added: clang/test/CodeGenCXX/ps-dllstorage-vtable-rtti.cpp Modified: clang/lib/CodeGen/ItaniumCXXABI.cpp Removed: clang/test/CodeGenCXX/ps4-dllstorage-vtable-rtti.cpp ################################################################################ diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp index 18acf7784f714..8427286dee887 100644 --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/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); diff --git a/clang/test/CodeGenCXX/ps-dllstorage-vtable-rtti.cpp b/clang/test/CodeGenCXX/ps-dllstorage-vtable-rtti.cpp new file mode 100644 index 0000000000000..377e579058ac1 --- /dev/null +++ b/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(); } diff --git a/clang/test/CodeGenCXX/ps4-dllstorage-vtable-rtti.cpp b/clang/test/CodeGenCXX/ps4-dllstorage-vtable-rtti.cpp deleted file mode 100644 index 5724e78617df9..0000000000000 --- a/clang/test/CodeGenCXX/ps4-dllstorage-vtable-rtti.cpp +++ /dev/null @@ -1,211 +0,0 @@ -// 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 -fhalf-no-semantic-interposition | 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 | FileCheck %s --check-prefixes=PS4,SCEI_WI -// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-scei-ps4 -emit-llvm -o - %s -fhalf-no-semantic-interposition | FileCheck %s --check-prefixes=PS4,SCEI_PS4 -// RUN: %clang_cc1 -I%S -fdeclspec -triple x86_64-sie-ps5 -emit-llvm -o - %s -fhalf-no-semantic-interposition | 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 ptr -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 ptr{{$}} -// 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(); -} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
