[MSVC compatibility][DLLEXPORT/DLLIMPORT] Allow dllexport/dllimport for local classes

[wolfgangp]

Summary

This is intended to address issue 56068.

MSVC seems to allow dllexport/dllimport on local classes. It also allows local classes that derive from dllexported/dllimported template classes. The resulting classes get external linkage. The utility of this is questionable, but clang should do the same in Microsoft mode.

[probinson]

I've looked at this but I'd like someone more in tune with MSVC behavior to review as well.

[hans]

Interesting! Do you have an example where this (local dllexport/import classes) comes up in practice?

MSVC will also allow the following:

namespace {
struct __declspec(dllexport) T {
    void foo();
};
}

whereas Clang will not, and I think Clang is making the right call there.

clang/lib/AST/Decl.cpp

492

We usually check ASTContext's getTargetInfo().getCXXABI().isMicrosoft()

[wolfgangp]

In D145271#4171267, @hans wrote:

Interesting! Do you have an example where this (local dllexport/import classes) comes up in practice?

A customer complained about the following code (I'm obscuring the class names) compiling with MSVC but
rejected by clang:

template <class T>
class __declspec(dllimport) A
{
};

void func()
{
  // local class with internal linkage
  class B: public A<B>
  {
  };
}

The code derives from various instantiations of A in several contexts and it's causing extra work for the customer to do something different in a local context.
While I do agree that clang has it right I think this is more a case of bug-compatibility with MSVC.

MSVC will also allow the following:

namespace {
struct __declspec(dllexport) T {
    void foo();
};
}

whereas Clang will not, and I think Clang is making the right call there.

Again, I do think clang makes the right call in all of these cases, but it's about MSVC compatibility, for better or worse.
I did also look at the anonymous namespace case above, but that's a bit more complicated, so I didn't address it this time.

clang/lib/AST/Decl.cpp

492

That would not work for Playstation, since it's not following the Microsoft ABI. The call to shouldDLLImport...(), checks the triple (it includes Windows Itanium, Windows MSVC and PS).
Perhaps we need a less obscure predicate to indicate source compatibility with MSVC as opposed to MS ABI compatibility.

[hans]

+mstorsjo for thoughts about Windows code, even if this might not apply to mingw.

In D145271#4172636, @wolfgangp wrote:

A customer complained about the following code (I'm obscuring the class names) compiling with MSVC but
rejected by clang:

template <class T>
class __declspec(dllimport) A
{
};

void func()
{
  // local class with internal linkage
  class B: public A<B>
  {
  };
}

Oh I see, it's not intentionally trying to dllimport/export a local class, the problem is really dllimport/exporting a class template instantiated with a local class as template argument.

The problem can be hit without inheritance too:

template <typename> struct __declspec(dllimport) Base { };

void f() {
  struct Local { };
  Base<Local> x;
}

error: 'Base<Local>' must have external linkage when declared 'dllimport'

It still seems like the export/import there is an accident since Base<f::Local> can't really be referenced from outside the file anyway.

Perhaps rather than giving Base<f::Local> external linkage to allow it to be imported/exported, the better fix would be to drop its dllimport/export attribute when instantiated with a local type?

[wolfgangp]

It still seems like the export/import there is an accident since Base<f::Local> can't really be referenced from outside the file anyway.

Perhaps rather than giving Base<f::Local> external linkage to allow it to be imported/exported, the better fix would be to drop its dllimport/export attribute when instantiated with a local type?

I like it. However, we would then not match MSVC's behavior in generating external definitions for the class (useless as they are). I suppose that's OK.

But on another note, would we then abandon matching MSVC on the following?

void func() {
  class __declspec(dllexport) A {
  };
}

This is currently accepted by MSVC, though arguably much less defensible than the above example.

[hans]

In D145271#4178837, @wolfgangp wrote:

It still seems like the export/import there is an accident since Base<f::Local> can't really be referenced from outside the file anyway.

Perhaps rather than giving Base<f::Local> external linkage to allow it to be imported/exported, the better fix would be to drop its dllimport/export attribute when instantiated with a local type?

I like it. However, we would then not match MSVC's behavior in generating external definitions for the class (useless as they are). I suppose that's OK.

But on another note, would we then abandon matching MSVC on the following?

void func() {
  class __declspec(dllexport) A {
  };
}

This is currently accepted by MSVC, though arguably much less defensible than the above example.

Right. Unless someone shows a compelling example why it's needed, I think we should continue to reject that.

clang/lib/AST/Decl.cpp

void LinkageComputer::mergeTemplateLV(
   // Merge information from the template arguments.  We ignore
   // template-argument visibility if we've got an explicit
   // instantiation with a visibility attribute.
   const TemplateArgumentList &templateArgs = spec->getTemplateArgs();
   LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs, computation);
   if (considerVisibility)
     LV.mergeVisibility(argsLV);
   LV.mergeExternalVisibility(argsLV);
+
+  // MSVC forces external linkage for explicit instantiations of template
+  // classes with dllexport/dllimport attributes.
+  if (spec->getASTContext().getTargetInfo().shouldDLLImportComdatSymbols())
+    if (spec->hasAttr<DLLExportAttr>() || spec->hasAttr<DLLImportAttr>())
+      LV.setLinkage(Linkage::ExternalLinkage);
 }
 
 /// Should we consider visibility associated with the template
 /// arguments and parameters of the given variable template
 /// specialization? As usual, follow class template specialization
 /// logic up to initialization.
 static bool shouldConsiderTemplateVisibility(
                                  const VarTemplateSpecializationDecl *spec,
if (Var->hasExternalStorage()) {
       return LV;
     }
 
     if (!Var->isStaticLocal())
       return LinkageInfo::none();
   }
 
   ASTContext &Context = D->getASTContext();
+
+  // MSVC allows dllexport/llimport on local classes and imparts external
+  // linkage to them.
+  if (Context.getTargetInfo().shouldDLLImportComdatSymbols()) {
+    if (const auto *Record = dyn_cast<CXXRecordDecl>(D))
+      if (Record->hasAttr<DLLExportAttr>() || Record->hasAttr<DLLImportAttr>())
+        return LinkageInfo();
+  }
+
   if (!Context.getLangOpts().CPlusPlus)
     return LinkageInfo::none();
 
   const Decl *OuterD = getOutermostFuncOrBlockContext(D);
   if (!OuterD || OuterD->isInvalidDecl())
     return LinkageInfo::none();
 
   LinkageInfo LV;

clang/test/SemaCXX/dllexport.cpp

struct __declspec(dllexport) PR23308 {
   void f(InternalLinkageType*);
 };
 void PR23308::f(InternalLinkageType*) {} // No error; we don't try to export f because it has internal linkage.
 
 //===----------------------------------------------------------------------===//
 // Classes with template base classes
 //===----------------------------------------------------------------------===//
 
+class Base {};
+class __declspec(dllexport) ExportedClass {};
+class __declspec(dllimport) ImportedClass {};
+
 template <typename T> class ClassTemplate {};
+#if not defined(MS) && not defined(WI)
+// expected-error@+4{{'ExportedClassTemplate<LocalCRTP>' must have external linkage when declared 'dllexport'}}
+// expected-error@+3{{'ExportedClassTemplate<ExportedLocalDerivedFromExportedTemplateCRTP>' must have external linkage when declared 'dllexport'}}
+// expected-error@+3{{'ImportedClassTemplate<ExportedLocalDerivedFromImportedTemplateCRTP>' must have external linkage when declared 'dllimport'}}
+#endif
 template <typename T> class __declspec(dllexport) ExportedClassTemplate {};
 template <typename T> class __declspec(dllimport) ImportedClassTemplate {};
 
 template <typename T> struct ExplicitlySpecializedTemplate { void func() {} };
 #ifdef MS
 // expected-note@+2{{class template 'ExplicitlySpecializedTemplate<int>' was explicitly specialized here}}
 #endif
 template <> struct ExplicitlySpecializedTemplate<int> { void func() {} };

 
 // Base class already instantiated with import attribute.
 struct __declspec(dllexport) DerivedFromExplicitlyImportInstantiatedTemplate : public ExplicitlyImportInstantiatedTemplate<int> {};
 
 template <typename T> struct ExplicitInstantiationDeclTemplateBase { void func() {} };
 extern template struct ExplicitInstantiationDeclTemplateBase<int>;
 struct __declspec(dllexport) DerivedFromExplicitInstantiationDeclTemplateBase : public ExplicitInstantiationDeclTemplateBase<int> {};
 
+void func() {
+  // MSVC propagates dllexport to derived classes even if they don't have external linkage.
+  class LocalDerivedFromExportedClass : public ExportedClass {};
+  class LocalDerivedFromExportedTemplate : public ExportedClassTemplate<int> {};
+#if not defined(MS) && not defined (WI)
+  // expected-note@+2{{in instantiation of template class 'ExportedClassTemplate<LocalCRTP>' requested here}}
+#endif
+  class LocalCRTP : public ExportedClassTemplate<LocalCRTP> {};
+  // MSVC allows exported classes in a local context.
+#if not defined(MS) && not defined (WI)
+  // expected-error@+10{{'ExportedLocalDerivedFromBase' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromTemplate' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromExportedClass' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromImportedClass' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromExportedTemplate' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromImportedTemplate' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalCRTP' must have external linkage when declared 'dllexport'}}
+  // expected-error@+10{{'ExportedLocalDerivedFromExportedTemplateCRTP' must have external linkage when declared 'dllexport'}}
+  // expected-error@+14{{'ExportedLocalDerivedFromImportedTemplateCRTP' must have external linkage when declared 'dllexport'}}
+#endif
+  class __declspec(dllexport) ExportedLocalDerivedFromBase : public Base {};
+  class __declspec(dllexport) ExportedLocalDerivedFromTemplate : public ClassTemplate<int> {};
+  class __declspec(dllexport) ExportedLocalDerivedFromExportedClass : public ExportedClass {};
+  class __declspec(dllexport) ExportedLocalDerivedFromImportedClass : public ImportedClass {};
+  class __declspec(dllexport) ExportedLocalDerivedFromExportedTemplate : public ExportedClassTemplate<int> {};
+  class __declspec(dllexport) ExportedLocalDerivedFromImportedTemplate : public ImportedClassTemplate<int> {};
+  class __declspec(dllexport) ExportedLocalCRTP : public ClassTemplate<ExportedLocalCRTP> {};
+  class __declspec(dllexport) ExportedLocalDerivedFromExportedTemplateCRTP :
+#if not defined(MS) && not defined (WI)
+  // expected-note@+2{{in instantiation of template class 'ExportedClassTemplate<ExportedLocalDerivedFromExportedTemplateCRTP>' requested here}}
+#endif
+    public ExportedClassTemplate<ExportedLocalDerivedFromExportedTemplateCRTP> {};
+  class __declspec(dllexport) ExportedLocalDerivedFromImportedTemplateCRTP :
+#if not defined(MS) && not defined (WI)
+  // expected-note@+2{{in instantiation of template class 'ImportedClassTemplate<ExportedLocalDerivedFromImportedTemplateCRTP>' requested here}}
+#endif
+    public ImportedClassTemplate<ExportedLocalDerivedFromImportedTemplateCRTP> {};
+}
 
 //===----------------------------------------------------------------------===//
 // Precedence
 //===----------------------------------------------------------------------===//
 
 // dllexport takes precedence over dllimport if both are specified.
 __attribute__((dllimport, dllexport))       extern int PrecedenceExternGlobal1A; // expected-warning{{'dllimport' attribute ignored}}
 __declspec(dllimport) __declspec(dllexport) extern int PrecedenceExternGlobal1B; // expected-warning{{'dllimport' attribute ignored}}

clang/test/SemaCXX/dllimport.cpp

 template<typename T> __declspec(dllimport) Internal InternalTypeVarTmpl; // expected-error{{'InternalTypeVarTmpl' must have external linkage when declared 'dllimport'}}
 #ifndef MS
 namespace    { template<typename T> __declspec(dllimport) int InternalVarTmpl; } // expected-error{{'(anonymous namespace)::InternalVarTmpl' must have external linkage when declared 'dllimport'}}
 #endif
 namespace ns { template<typename T> __declspec(dllimport) int ExternalVarTmpl; }
 
 template<typename T> __declspec(dllimport) auto InternalAutoTypeVarTmpl = Internal(); // expected-error{{definition of dllimport data}} // expected-error{{'InternalAutoTypeVarTmpl' must have external linkage when declared 'dllimport'}}
 
-
 template<typename T> int VarTmpl;
 template<typename T> __declspec(dllimport) int ImportedVarTmpl;
 
 // Import implicit instantiation of an imported variable template.
 int useVarTmpl() { return ImportedVarTmpl<ImplicitInst_Imported>; }
 
 // Import explicit instantiation declaration of an imported variable template.
 extern template int ImportedVarTmpl<ExplicitDecl_Imported>;

 template <typename T> struct ExpliciallySpecializedClassTemplate {};
 template <> struct __declspec(dllimport) ExpliciallySpecializedClassTemplate<int> { void f() {} };
 
 
 //===----------------------------------------------------------------------===//
 // Classes with template base classes
 //===----------------------------------------------------------------------===//
 
+class Base {};
+class __declspec(dllexport) ExportedClass {};
+class __declspec(dllimport) ImportedClass {};
+
+#ifdef GNU
+// expected-error@+2{{'ExportedClassTemplate<ImportedLocalDerivedFromExportedTemplateCRTP>' must have external linkage when declared 'dllexport'}}
+#endif
 template <typename T> class __declspec(dllexport) ExportedClassTemplate {};
 
+#ifdef GNU
+// expected-error@+3{{'ImportedClassTemplate<LocalCRTP>' must have external linkage when declared 'dllimport'}}
+// expected-error@+2{{'ImportedClassTemplate<ImportedLocalDerivedFromImportedTemplateCRTP>' must have external linkage when declared 'dllimport'}}
+#endif
 template <typename T> class __declspec(dllimport) ImportedClassTemplate {};
 
 // ClassTemplate<int> gets imported.
 class __declspec(dllimport) DerivedFromTemplate : public ClassTemplate<int> {};
 
 // ClassTemplate<int> is already imported.
 class __declspec(dllimport) DerivedFromTemplate2 : public ClassTemplate<int> {};
 

 
 // Base class already instantiated with import attribute.
 struct __declspec(dllimport) DerivedFromExplicitlyImportInstantiatedTemplate : public ExplicitlyImportInstantiatedTemplate<int> {};
 
 template <typename T> struct ExplicitInstantiationDeclTemplateBase { void func() {} };
 extern template struct ExplicitInstantiationDeclTemplateBase<int>;
 struct __declspec(dllimport) DerivedFromExplicitInstantiationDeclTemplateBase : public ExplicitInstantiationDeclTemplateBase<int> {};
 
+void func() {
+  // MSVC propagates dllimport to derived classes even if they don't have external linkage.
+  class LocalDerivedFromImportedClass : public ImportedClass {};
+  class LocalDerivedFromImportedTemplate : public ImportedClassTemplate<int> {};
+#ifdef GNU
+  // expected-note@+2{{in instantiation of template class 'ImportedClassTemplate<LocalCRTP>' requested here}}
+#endif
+  class LocalCRTP : public ImportedClassTemplate<LocalCRTP> {};
+  // MSVC allows imported classes in a local context.
+#ifdef GNU
+  // expected-error@+12{{'ImportedLocalDerivedFromBase' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromTemplate' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromExportedClass' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromImportedClass' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromExportedTemplate' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromImportedTemplate' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalCRTP' must have external linkage when declared 'dllimport'}}
+  // expected-error@+12{{'ImportedLocalDerivedFromExportedTemplateCRTP' must have external linkage when declared 'dllimport'}}
+  // expected-error@+13{{'ImportedLocalDerivedFromImportedTemplateCRTP' must have external linkage when declared 'dllimport'}}
+  // expected-note@+11{{in instantiation of template class 'ExportedClassTemplate<ImportedLocalDerivedFromExportedTemplateCRTP>' requested here}}
+  // expected-note@+12{{in instantiation of template class 'ImportedClassTemplate<ImportedLocalDerivedFromImportedTemplateCRTP>' requested here}}
+#endif
+  class __declspec(dllimport) ImportedLocalDerivedFromBase : public Base {};
+  class __declspec(dllimport) ImportedLocalDerivedFromTemplate : public ClassTemplate<int> {};
+  class __declspec(dllimport) ImportedLocalDerivedFromExportedClass : public ExportedClass {};
+  class __declspec(dllimport) ImportedLocalDerivedFromImportedClass : public ImportedClass {};
+  class __declspec(dllimport) ImportedLocalDerivedFromExportedTemplate : public ExportedClassTemplate<int> {};
+  class __declspec(dllimport) ImportedLocalDerivedFromImportedTemplate : public ImportedClassTemplate<int> {};
+  class __declspec(dllimport) ImportedLocalCRTP : public ClassTemplate<ImportedLocalCRTP> {};
+  class __declspec(dllimport) ImportedLocalDerivedFromExportedTemplateCRTP :
+    public ExportedClassTemplate<ImportedLocalDerivedFromExportedTemplateCRTP> {};
+  class __declspec(dllimport) ImportedLocalDerivedFromImportedTemplateCRTP :
+    public ImportedClassTemplate<ImportedLocalDerivedFromImportedTemplateCRTP> {};
+}
+
 //===----------------------------------------------------------------------===//
 // Lambdas
 //===----------------------------------------------------------------------===//
 // The MS ABI doesn't provide a stable mangling for lambdas, so they can't be imported or exported.
 #if defined(MS) || defined(WI)
 // expected-error@+4{{lambda cannot be declared 'dllimport'}}
 #else
 // expected-warning@+2{{'dllimport' attribute ignored on inline function}}
 #endif
 auto Lambda = []() __declspec(dllimport) -> bool { return true; };