[windows-itanium] handle dllimport/export code paths separately and share with PS4

[bd1976llvm]

Summary

Similar to Windows Itanium, PS4 is also an Itanium C++ ABI variant and shares the goal of source code compatibility with Microsoft code that uses dllimport/export.

This change introduces a new function to determine from the triple if an environment aims for compatibility with MS w.r.t these attributes and guards the relevant code paths using that function.

[compnerd]

Thanks, this is definitely a nice cleanup. I would say isDLLStorageEnvironment is a better predicate. Would be nice to get @rnk's opinion (and @mstorsjo, if they are interested).

[rnk]

Will there be any need to check this in LLVM? If not, I would prefer to put this on clang::TargetInfo.

clang/lib/CodeGen/ItaniumCXXABI.cpp

3201

CGM also has a getTarget() method to get TargetInfo if the predicate moves there.

[mstorsjo]

In D90299#2362220, @compnerd wrote:

I would say isDLLStorageEnvironment is a better predicate.

That sounds less specific to me, and would imply that it also would cover mingw targets (which it doesn't).

The current suggested name, isDLLImportExportMSCompatible, is more specific, but when I first read it my reaction was that mingw's dllexport/import (in the general cases) for sure works the same as for MSVC - if you consider C functions. So if the name can make it clearer that this covers C++ cases, it'd be more intuitive to me, even though I'm sure it'd become even more of a mouthful then...

[rnk]

I agree, the name could be improved. For prior art, see /Zc:dllexportInlines-, which disables some (but not all) of the dllexport/import semantics in clang-cl.

What we are really talking about here is what should be done when importing or exporting C++ constructs with vague linkage (inline functions, templates). That's what all the affected code has in common. Clang doesn't often use the terminology of vague linkage, so maybe it would be better to use the name comdat somehow.

How about TargetInfo::shouldDLLImportComdatSymbols? You could add in "export" (shouldDLLImportExportComdatSymbols), but GCC also exports comdat things sometimes, and in the cases where this predicate controls export behavior, it's pretty clear that it needs to be done to support the side which would import the symbol (explicit template instantiations).

[bd1976llvm]

In D90299#2362895, @rnk wrote:

I agree, the name could be improved. For prior art, see /Zc:dllexportInlines-, which disables some (but not all) of the dllexport/import semantics in clang-cl.

What we are really talking about here is what should be done when importing or exporting C++ constructs with vague linkage (inline functions, templates). That's what all the affected code has in common. Clang doesn't often use the terminology of vague linkage, so maybe it would be better to use the name comdat somehow.

How about TargetInfo::shouldDLLImportComdatSymbols? You could add in "export" (shouldDLLImportExportComdatSymbols), but GCC also exports comdat things sometimes, and in the cases where this predicate controls export behavior, it's pretty clear that it needs to be done to support the side which would import the symbol (explicit template instantiations).

Thanks for the comments. Naming is always hard! I will adopt @rnk's shouldDLLImportComdatSymbols unless anyone has an objection?

[bd1976llvm]

Addressed review comments.

[bd1976llvm]

clang/lib/CodeGen/ItaniumCXXABI.cpp

3201

Thanks!

[rnk]

Please add tests for the common cases:

• exported class with inline methods
• exported explicit class template instantiation
• imported class with inline methods
• imported extern class template declaration

You might be able to piggyback on any windows-itanium dllimport tests, if they exist, since the LLVM IR should match.

[bd1976llvm]

Add tests and prevent SemaTemplate and ItaniumCXXABI code paths from executing for PS4 (for now).

I have had to be a bit more cautious as to which code paths I enable for PS4 (vs the initial diff) as I realised that we need to do more testing downstream to be sure the patches in SemaTemplate and ItaniumCXXABI will work with the propriety schemes that we are currently using. For the SemaTemplate patches I have added an exception for PS4 (that I expect to remove once we have completed further testing) and tests to show that PS4 doesn't follow Windows Itanium here. For the ItaniumCXXABI patches, I have left the patches as specific to Windows Itanium. Those patches handle how dllimport/export interacts with RTTI data; on PS4 we currently have a custom scheme for this. I am hopeful that we can use those patches in the future and I am working on verifying this internally but there is some chance that we might have to use a PS4 specific scheme for this. I also discovered that RTTI dllexport/import tests currently assert with the upstream PS4 target - so I couldn't write tests to show that PS4 doesn't follow the Windows Itanium behaviour here.

There were existing LIT tests covering all of the code paths. I have added a run line with the PS4 triple to the appropriate ones.

The patches in SemaDecl.cpp, SemaDeclAttr.cpp and SemaDeclCXX.cpp are covered by the tests:

• Sema/dllimport.c
• SemaCXX/dllexport.cpp
• SemaCXX/dllimport.cpp
• CodeGenCXX/windows-itanium-dllexport.cpp

The patches in SemaTemplate.cpp are covered by:

• CodeGenCXX/windows-implicit-dllexport-template-specialization.cpp
• CodeGenCXX/windows-itanium-dllexport.cpp

The code changes in ItaniumCXXABI.cpp are covered by:

• CodeGenCXX/windows-itanium-type-info.cpp
• CodeGenCXXwindows-itanium-exceptions.cpp

[bd1976llvm]

I think this needs a re-review as I have deviated from the original simple approach.

[bd1976llvm]

Let me know if you think that more tests need adding. I have added support for LIT based run tests to the build recipe (https://reviews.llvm.org/D88124) I can use this to add any run tests that might be useful for this review.

[compnerd]

clang/include/clang/Basic/TargetInfo.h

1096 ↗

(On Diff #303285)

I don't think its source compatibility, but rather semantic compatibility. -fdeclspec gives you the source level compatibility - it can process the source, just may not do the same thing.

clang/lib/Sema/SemaDecl.cpp

6513 ↗

(On Diff #303285)

I think that renaming IsMicrosoft is now warranted.

clang/lib/Sema/SemaTemplate.cpp

9765

Please update the comment (I think we can entirely remove it now given the new name). This probably fits better in the definition of shouldImportDLLImportComdatSymbols now as the condition is completely different now.

[bd1976llvm]

Thanks for the comments. I have updated the diff.

Just to explain a bit more about why I haven't been able to adopt all the Windows Itanium code paths for PS4: I am confident that the Windows Itanium code is correct for PS4 and it passes all of our downstream testing. However, there is a very strict backwards compatibility requirement that is preventing me from using all of these code paths for PS4 (see https://www.youtube.com/watch?v=ATOfCfM9hSg for a talk about ABI compatibility difficulties). For the SemaTemplate paths I am still hopeful that we can adopt this behaviour. For the ItaniumCXXABI paths I think we can eventually adopt these in the future but for the next few years we will need to use a PS4 specific scheme for handling RTTI and vtables with dllimport/export. I will put a WIP review shortly for this scheme.

clang/include/clang/Basic/TargetInfo.h

1096 ↗

(On Diff #303285)

Thanks.

clang/lib/Sema/SemaDecl.cpp

6513 ↗

(On Diff #303285)

What would be better though? The MS ABI is the standard for the dllimport/export behaviour. Therefore, I think IsMicrosoft is quite a good name and it fits well with the existing comments.

clang/lib/Sema/SemaTemplate.cpp

9765

The comment seems helpful to me though, no? Also, this comment is rather tied to the matching comment on L9782. Since the MS ABI is standard for dllimport/export behaviour and MinGW is the outlier (in that it does it's own thing, a bit, as far as C++ dllimport/export goes) it seems reasonable that these two are mentioned?

[bd1976llvm]

Updated comment to state that "semantic compatibility" is the goal.

[compnerd]

clang/lib/Sema/SemaDecl.cpp

6513 ↗

(On Diff #303285)

Then IsMicrosoftABI sounds better :). The problem with the current name is that its confusing between the Microsoft environment and the Microsoft ABI.

clang/lib/Sema/SemaTemplate.cpp

9765

I think I would be okay with retaining the comment with the initial clause In the MS ABI, removed.

[bd1976llvm]

Addressed review comments.

[bd1976llvm]

clang/lib/Sema/SemaDecl.cpp

6513 ↗

(On Diff #303285)

Done. Thanks.

clang/lib/Sema/SemaTemplate.cpp

9765

Done. Thanks!

[bd1976llvm]

This comment was removed by bd1976llvm.

clang/lib/CodeGen/ItaniumCXXABI.cpp

if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
     // 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))
-      return IsDLLImport && !CGM.getTriple().isWindowsItaniumEnvironment()
+      return IsDLLImport && !CGM.getTriple().isDLLImportExportMSCompatible()
                  ? false
                  : true;
 
     if (IsDLLImport)
       return true;
   }
 
   return false;
case ExternalLinkage:
     // to be used for exception handling. Give it linkonce_odr linkage.
     if (!CGM.getLangOpts().RTTI)
       return llvm::GlobalValue::LinkOnceODRLinkage;
 
     if (const RecordType *Record = dyn_cast<RecordType>(Ty)) {
       const CXXRecordDecl *RD = cast<CXXRecordDecl>(Record->getDecl());
       if (RD->hasAttr<WeakAttr>())
         return llvm::GlobalValue::WeakODRLinkage;
-      if (CGM.getTriple().isWindowsItaniumEnvironment())
+      if (CGM.getTriple().isDLLImportExportMSCompatible())
         if (RD->hasAttr<DLLImportAttr>() &&
             ShouldUseExternalRTTIDescriptor(CGM, Ty))
           return llvm::GlobalValue::ExternalLinkage;
       // MinGW always uses LinkOnceODRLinkage for type info.
       if (RD->isDynamicClass() &&
           !CGM.getContext()
                .getTargetInfo()
                .getTriple()
llvm::Constant *ItaniumRTTIBuilder::BuildTypeInfo(QualType Ty) {
   else if (CXXABI.classifyRTTIUniqueness(Ty, Linkage) ==
            ItaniumCXXABI::RUK_NonUniqueHidden)
     llvmVisibility = llvm::GlobalValue::HiddenVisibility;
   else
     llvmVisibility = CodeGenModule::GetLLVMVisibility(Ty->getVisibility());
 
   llvm::GlobalValue::DLLStorageClassTypes DLLStorageClass =
       llvm::GlobalValue::DefaultStorageClass;
-  if (CGM.getTriple().isWindowsItaniumEnvironment()) {
+  if (CGM.getTriple().isDLLImportExportMSCompatible()) {
     auto RD = Ty->getAsCXXRecordDecl();
     if (RD && RD->hasAttr<DLLExportAttr>())
       DLLStorageClass = llvm::GlobalValue::DLLExportStorageClass;
   }
 
   return BuildTypeInfo(Ty, Linkage, llvmVisibility, DLLStorageClass);
 }
 

clang/lib/Sema/SemaDeclAttr.cpp

DLLExportAttr *Sema::mergeDLLExportAttr(Decl *D,
   if (D->hasAttr<DLLExportAttr>())
     return nullptr;
 
   return ::new (Context) DLLExportAttr(Context, CI);
 }
 
 static void handleDLLAttr(Sema &S, Decl *D, const ParsedAttr &A) {
   if (isa<ClassTemplatePartialSpecializationDecl>(D) &&
-      (S.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-       S.Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+      (S.Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible())) {
     S.Diag(A.getRange().getBegin(), diag::warn_attribute_ignored) << A;
     return;
   }
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     if (FD->isInlined() && A.getKind() == ParsedAttr::AT_DLLImport &&
-        !(S.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-          S.Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+        !(S.Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible())) {
       // MinGW doesn't allow dllimport on inline functions.
       S.Diag(A.getRange().getBegin(), diag::warn_attribute_ignored_on_inline)
           << A;
       return;
     }
   }
 
   if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
-    if ((S.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-         S.Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment()) &&
+    if ((S.Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible()) &&
         MD->getParent()->isLambda()) {
       S.Diag(A.getRange().getBegin(), diag::err_attribute_dll_lambda) << A;
       return;
     }
   }
 
   Attr *NewAttr = A.getKind() == ParsedAttr::AT_DLLExport
                       ? (Attr *)S.mergeDLLExportAttr(D, A)

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::checkClassLevelCodeSegAttribute(CXXRecordDecl *Class) {
   }
 }
 
 /// Check class-level dllimport/dllexport attribute.
 void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
   Attr *ClassAttr = getDLLAttr(Class);
 
   // MSVC inherits DLL attributes to partial class template specializations.
-  if ((Context.getTargetInfo().getCXXABI().isMicrosoft() || 
-       Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment()) && !ClassAttr) {
+  if (Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible() &&
+      !ClassAttr) {
     if (auto *Spec = dyn_cast<ClassTemplatePartialSpecializationDecl>(Class)) {
       if (Attr *TemplateAttr =
               getDLLAttr(Spec->getSpecializedTemplate()->getTemplatedDecl())) {
         auto *A = cast<InheritableAttr>(TemplateAttr->clone(getASTContext()));
         A->setInherited(true);
         ClassAttr = A;
       }
     }
   }
 
   if (!ClassAttr)
     return;
 
   if (!Class->isExternallyVisible()) {
     Diag(Class->getLocation(), diag::err_attribute_dll_not_extern)
         << Class << ClassAttr;
     return;
   }
 
-  if ((Context.getTargetInfo().getCXXABI().isMicrosoft() || 
-       Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment()) &&
+  if (Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible() &&
       !ClassAttr->isInherited()) {
     // Diagnose dll attributes on members of class with dll attribute.
     for (Decl *Member : Class->decls()) {
       if (!isa<VarDecl>(Member) && !isa<CXXMethodDecl>(Member))
         continue;
       InheritableAttr *MemberAttr = getDLLAttr(Member);
       if (!MemberAttr || MemberAttr->isInherited() || Member->isInvalidDecl())
         continue;
for (Decl *Member : Class->decls()) {
     if (MD) {
       // Don't process deleted methods.
       if (MD->isDeleted())
         continue;
 
       if (MD->isInlined()) {
         // MinGW does not import or export inline methods. But do it for
         // template instantiations.
-        if (!Context.getTargetInfo().getCXXABI().isMicrosoft() &&
-            !Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment() &&
+        if (!Context.getTargetInfo()
+                 .getTriple()
+                 .isDLLImportExportMSCompatible() &&
             TSK != TSK_ExplicitInstantiationDeclaration &&
             TSK != TSK_ExplicitInstantiationDefinition)
           continue;
 
         // MSVC versions before 2015 don't export the move assignment operators
         // and move constructor, so don't attempt to import/export them if
         // we have a definition.
         auto *Ctor = dyn_cast<CXXConstructorDecl>(MD);

clang/lib/Sema/SemaTemplate.cpp

if (Def) {
     // TSK_ExplicitInstantiationDefinition
     if (Old_TSK == TSK_ExplicitInstantiationDeclaration &&
         (TSK == TSK_ExplicitInstantiationDefinition ||
          DLLImportExplicitInstantiationDef)) {
       // FIXME: Need to notify the ASTMutationListener that we did this.
       Def->setTemplateSpecializationKind(TSK);
 
       if (!getDLLAttr(Def) && getDLLAttr(Specialization) &&
-          (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-           Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+          Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible()) {
         // In the MS ABI, an explicit instantiation definition can add a dll
         // attribute to a template with a previous instantiation declaration.
         // MinGW doesn't allow this.
         auto *A = cast<InheritableAttr>(
             getDLLAttr(Specialization)->clone(getASTContext()));
         A->setInherited(true);
         Def->addAttr(A);
         dllExportImportClassTemplateSpecialization(*this, Def);
       }
     }
 
     // Fix a TSK_ImplicitInstantiation followed by a
     // TSK_ExplicitInstantiationDefinition
     bool NewlyDLLExported =
         !PreviouslyDLLExported && Specialization->hasAttr<DLLExportAttr>();
     if (Old_TSK == TSK_ImplicitInstantiation && NewlyDLLExported &&
-        (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-         Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+        Context.getTargetInfo().getTriple().isDLLImportExportMSCompatible()) {
       // In the MS ABI, an explicit instantiation definition can add a dll
       // attribute to a template with a previous implicit instantiation.
       // MinGW doesn't allow this. We limit clang to only adding dllexport, to
       // avoid potentially strange codegen behavior.  For example, if we extend
       // this conditional to dllimport, and we have a source file calling a
       // method on an implicitly instantiated template class instance and then
       // declaring a dllimport explicit instantiation definition for the same
       // template class, the codegen for the method call will not respect the

llvm/include/llvm/ADT/Triple.h

public:
 
   /// Tests whether the target is the PS4 CPU
   bool isPS4CPU() const {
     return getArch() == Triple::x86_64 &&
            getVendor() == Triple::SCEI &&
            getOS() == Triple::PS4;
   }
 
+  bool isDLLImportExportMSCompatible() const {
+    return isWindowsMSVCEnvironment() || isWindowsItaniumEnvironment() ||
+           isPS4CPU();
+  }
+
   /// Tests whether the target is the PS4 platform
   bool isPS4() const {
     return getVendor() == Triple::SCEI &&
            getOS() == Triple::PS4;
   }
 
   /// Tests whether the target is Android
   bool isAndroid() const { return getEnvironment() == Triple::Android; }