[Sema] Respect DLL attributes more faithfully

[smeenai]

Summary

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior for
exporting.

[compnerd]

lib/Sema/SemaTemplate.cpp

7686

The fragment doesn't make sense to me. Perhaps needs a bit more context?

[smeenai]

lib/Sema/SemaTemplate.cpp

7686

Do you mean the comment specifically?

[hans]

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

[smeenai]

In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ. With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

[hans]

In D26657#596897, @smeenai wrote:

In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

[smeenai]

In D26657#597523, @hans wrote:

In D26657#596897, @smeenai wrote:

In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

Huh. Weird stuff.

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

So is the conclusion that this is probably safe as is, or should I limit it to dllexport specifically?

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

Ah. We put dllimport on the explicit template instantiation declarations in the header (all the instances of _LIBCPP_EXTERN_TEMPLATE2 in that file). Those get paired with the dllexport on the corresponding instantiation definitions. If you're compiling the library, the declarations have no DLL attribute, and the definitions have dllexport. If you're referencing the headers from outside the library, the declarations get the dllimport.

[hans]

In D26657#597859, @smeenai wrote:

In D26657#597523, @hans wrote:

In D26657#596897, @smeenai wrote:

In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

Huh. Weird stuff.

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

So is the conclusion that this is probably safe as is, or should I limit it to dllexport specifically?

I think we should limit it do dllexport if we can't fix the dllimport case.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

Ah. We put dllimport on the explicit template instantiation declarations in the header (all the instances of _LIBCPP_EXTERN_TEMPLATE2 in that file). Those get paired with the dllexport on the corresponding instantiation definitions. If you're compiling the library, the declarations have no DLL attribute, and the definitions have dllexport. If you're referencing the headers from outside the library, the declarations get the dllimport.

[rnk]

lib/Sema/SemaTemplate.cpp

7687–7690

Is this the right logic in the case of Old_TSK == TSK_Implicit && TSK == TSK_Explicit? We don't want a behavior change when DLLImport is not involved. A test for that case would be good.

[smeenai]

lib/Sema/SemaTemplate.cpp

7687–7690

I'm actually concerned about the dllexport case and not the dllimport case, after my discussion with @hans. I'm updating this diff to that effect.

[smeenai]

Limiting to dllexport after discussion with @hans

[majnemer]

lib/Sema/SemaTemplate.cpp

7697–7699

Please do not do this. The canonical location for comments in LLVM are the comments themselves.

We have moved the location of the review system and I can easily believe that it will move yet again.

[smeenai]

Moving explanation to comment instead of referencing Phabricator

[hans]

lib/Sema/SemaTemplate.cpp

7687–7690

I think what Reid is alluding to is regardless of dll attributes, you're adding another OR-clause to this if condition which will cause us to e.g. do Def->setTemplateSpecializationKind below, so something is changing in how we handle implicit instantiation followed by explicit instantiation def in general, not just for dllexport which might not be what we want?

[smeenai]

lib/Sema/SemaTemplate.cpp

7687–7690

That's a fair point. I'll modify this to address that.

[smeenai]

Addressing comments

[smeenai]

Typo in comment

[smeenai]

General coding style question. Over here, I'm creating a local helper function. However, that helper needs to access member functions of Sema, which is why I made it a private member function right now, which also unfortunately makes the change somewhat non-local (since the header file needs to be modified as well). I can think of two alternatives:

• Define a local helper lambda (which will capture this) instead of a private member function.
• Define a local static helper function and pass the Sema instance as a parameter so that it can call member functions on it.

Would either of those be preferable to what I currently have? I'm pretty new when it comes to llvm/clang changes, so stylistic feedback is greatly appreciated.

[smeenai]

Ping.

[hans]

Apologies for the delay. I was out last week.

In D26657#602083, @smeenai wrote:

General coding style question. Over here, I'm creating a local helper function. However, that helper needs to access member functions of Sema, which is why I made it a private member function right now, which also unfortunately makes the change somewhat non-local (since the header file needs to be modified as well). I can think of two alternatives:

• Define a local helper lambda (which will capture this) instead of a private member function.
• Define a local static helper function and pass the Sema instance as a parameter so that it can call member functions on it.

Would either of those be preferable to what I currently have? I'm pretty new when it comes to llvm/clang changes, so stylistic feedback is greatly appreciated.

I usually prefer a static helper function and passing the Sema instance if it's possiblem, but that requires the Sema members we need to access to be public. If that's not the case, adding the helper as a member function is fine.

include/clang/Sema/Sema.h

7494

Nit: I think /// implies \brief, so we don't usually include it.

7495

I'd suggest making the function name more specific. It obviously doesn't apply to all DLL attributes, but only class templates.

Also, the "ActOn" methods in Sema are used as an interface to be called by the parser, so I would avoid that prefix.

Naming is hard :-/

Maybe checkDllExportedTemplateSpecialization

lib/Sema/SemaTemplate.cpp

7439

This function only applies to dllexport, not dllimport, so it would be good if the name reflected that, and maybe we could also add an assert to check for it.

7710

Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS ABI is sufficient?

[hans]

include/clang/Sema/Sema.h

7495

also it should be in the private: section since it's just a helper. And maybe the "check" part is unnecessary? dllExportClassTemplateSpecialization might work.

[smeenai]

include/clang/Sema/Sema.h

7494

Cool, will drop.

7495

Good point about the prefix. I'll do the rename. I intended to make it private; if it's in the wrong part of the header, that's an accident on my part :)

lib/Sema/SemaTemplate.cpp

7439

It's called in two places (the refactored original call for explicit instantiation declaration followed by explicit instantiation definition, and my new call for implicit instantiation followed by explicit instantiation definition). The dllexport guarantee only applies to the second one, right? I'll come up with a better name based on your suggestions in the other comment.

7710

windows-itanium in general tries to stick to MSVC semantics for dllexport/import annotations (unlike Cygwin and MinGW which kinda do their own thing). This is consistent with the conditional for the previous case (lines 7691 to 7693 in this diff).

[hans]

lib/Sema/SemaTemplate.cpp

7439

You're right, my mistake.

7710

Oh I see, this seems to be a new thing, starting with e.g. r284288.

Seems fine then, but I'm a little worried that we're adding another variable into the matrix here. IIRC, we key dll attribute behaviour off getCXXABI().isMicrosoft() in lots of places.

[smeenai]

Addressing comments

lib/Sema/SemaTemplate.cpp

7710

You're right; the current situation isn't ideal. I can clean that up in a follow-up.

[hans]

lgtm

lib/Sema/SemaTemplate.cpp

7710

Sounds good.

include/clang/Sema/Sema.h

void ActOnTypedefedProtocols(SmallVectorImpl<Decl *> &ProtocolRefs,
                                IdentifierInfo *SuperName,
                                SourceLocation SuperLoc);
 
   Decl *ActOnCompatibilityAlias(
                     SourceLocation AtCompatibilityAliasLoc,
                     IdentifierInfo *AliasName,  SourceLocation AliasLocation,
                     IdentifierInfo *ClassName, SourceLocation ClassLocation);
 
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+                    InheritableAttr *Attr);
+
   bool CheckForwardProtocolDeclarationForCircularDependency(
     IdentifierInfo *PName,
     SourceLocation &PLoc, SourceLocation PrevLoc,
     const ObjCList<ObjCProtocolDecl> &PList);
 
   Decl *ActOnStartProtocolInterface(
                     SourceLocation AtProtoInterfaceLoc,
                     IdentifierInfo *ProtocolName, SourceLocation ProtocolLoc,

lib/Sema/SemaTemplate.cpp

for (NestedNameSpecifier *NNS = SS.getScopeRep(); NNS;
        NNS = NNS->getPrefix())
     if (const Type *T = NNS->getAsType())
       if (isa<TemplateSpecializationType>(T))
         return true;
 
   return false;
 }
 
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+                        InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should
+  // never be any delayed exported classes to worry about.
+  assert(DelayedDllExportClasses.empty() &&
+         "delayed exports present at explicit instantiation");
+  checkClassLevelDLLAttribute(Def);
+  referenceDLLExportedClassMethods();
+
+  // Propagate attribute to base class templates.
+  for (auto &B : Def->bases()) {
+    if (auto *BT = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+            B.getType()->getAsCXXRecordDecl()))
+      propagateDLLAttrToBaseClassTemplate(Def, Attr, BT, B.getLocStart());
+  }
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
                                  SourceLocation ExternLoc,
                                  SourceLocation TemplateLoc,
                                  unsigned TagSpec,
                                  SourceLocation KWLoc,
                                  const CXXScopeSpec &SS,
Sema::ActOnExplicitInstantiation(Scope *S,
   // Instantiate the members of this class template specialization.
   Def = cast_or_null<ClassTemplateSpecializationDecl>(
                                        Specialization->getDefinition());
   if (Def) {
     TemplateSpecializationKind Old_TSK = Def->getTemplateSpecializationKind();
     // Fix a TSK_ExplicitInstantiationDeclaration followed by a
     // 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())) {
         // 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);
-
-        // We reject explicit instantiations in class scope, so there should
-        // never be any delayed exported classes to worry about.
-        assert(DelayedDllExportClasses.empty() &&
-               "delayed exports present at explicit instantiation");
-        checkClassLevelDLLAttribute(Def);
-        referenceDLLExportedClassMethods();
-
-        // Propagate attribute to base class templates.
-        for (auto &B : Def->bases()) {
-          if (auto *BT = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
-                  B.getType()->getAsCXXRecordDecl()))
-            propagateDLLAttrToBaseClassTemplate(Def, A, BT, B.getLocStart());
-        }
-      }
+        ActOnDLLAttr(Def, A);
+
+    // Fix a TSK_ImplicitInstantiation followed by a
+    // TSK_ExplicitInstantiationDefinition
+    if (Old_TSK == TSK_ImplicitInstantiation &&
+        Specialization->hasAttr<DLLExportAttr>() &&
+        (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+         Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+      // In the MS ABI, an explicit instantiation definition can add a dll
+      // attribute to a template with a previous implicit instantiation.
+      // or 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 dllimport, while it will with cl. The Def will
+      // already have the DLL attribute, since the Def and Specialization will
+      // be the same in the case of Old_TSK == TSK_ImplicitInstantiation, and we
+      // already added the attribute to the Specialization; we just need to make
+      // it take effect.
+      assert(Def == Specialization &&
+             "Def and Specialization should match for implicit instantiation");
+      auto *A = cast<InheritableAttr>(Def->getAttr<DLLExportAttr>());
+      ActOnDLLAttr(Def, A);
     }
 
     // Set the template specialization kind. Make sure it is set before
     // instantiating the members which will trigger ASTConsumer callbacks.
     Specialization->setTemplateSpecializationKind(TSK);
     InstantiateClassTemplateSpecializationMembers(TemplateNameLoc, Def, TSK);
   } else {
 

test/CodeGenCXX/dllexport.cpp

 template <typename T> struct ExplicitInstantiationDeclExportedDefTemplate { void f() {} ExplicitInstantiationDeclExportedDefTemplate() {} };
 extern template struct ExplicitInstantiationDeclExportedDefTemplate<int>;
 template struct __declspec(dllexport) ExplicitInstantiationDeclExportedDefTemplate<int>;
 USEMEMFUNC(ExplicitInstantiationDeclExportedDefTemplate<int>, f);
 // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?f@?$ExplicitInstantiationDeclExportedDefTemplate@H@@QAEXXZ"
 // M32-DAG: define weak_odr dllexport x86_thiscallcc %struct.ExplicitInstantiationDeclExportedDefTemplate* @"\01??0?$ExplicitInstantiationDeclExportedDefTemplate@H@@QAE@XZ"
 // G32-DAG: define weak_odr x86_thiscallcc void @_ZN44ExplicitInstantiationDeclExportedDefTemplateIiE1fEv
 
+template <typename T> struct ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate<int> ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) ImplicitInstantiationExplicitInstantiationDefExportedTemplate<int>;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate<int>, f);
+// M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?f@?$ImplicitInstantiationExplicitInstantiationDefExportedTemplate@H@@QAEXXZ"
+// G32-DAG: define weak_odr x86_thiscallcc void @_ZN61ImplicitInstantiationExplicitInstantiationDefExportedTemplateIiE1fEv
+
 namespace { struct InternalLinkageType {}; }
 struct __declspec(dllexport) PR23308 {
   void f(InternalLinkageType*);
 };
 void PR23308::f(InternalLinkageType*) {}
 long use(PR23308* p) { p->f(nullptr); }
 // M32-DAG: define internal x86_thiscallcc void @"\01?f@PR23308@@QAEXPAUInternalLinkageType@?A@@@Z"
 

test/CodeGenCXX/windows-itanium-dllexport.cpp

 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIiE1fEv
 
 extern template class c<char>;
 template class __declspec(dllexport) c<char>;
 
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c<double> g;
+template class __declspec(dllexport) c<double>;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv