[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

7669 ↗

(On Diff #77939)

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

[smeenai]

lib/Sema/SemaTemplate.cpp

7669 ↗

(On Diff #77939)

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

7670–7673 ↗

(On Diff #77939)

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

7670–7673 ↗

(On Diff #77939)

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

7683–7685 ↗

(On Diff #78322)

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

7670–7673 ↗

(On Diff #77939)

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

7670–7673 ↗

(On Diff #77939)

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 ↗

(On Diff #78621)

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

7495 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

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

[hans]

include/clang/Sema/Sema.h

7495 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

Cool, will drop.

7495 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

You're right, my mistake.

7710 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

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 ↗

(On Diff #78621)

Sounds good.

cfe/trunk/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;
 }
 
+/// Make a dllexport or dllimport attr on a class template specialization take
+/// effect.
+static void dllExportImportClassTemplateSpecialization(
+    Sema &S, ClassTemplateSpecializationDecl *Def) {
+  auto *A = cast_or_null<InheritableAttr>(getDLLAttr(Def));
+  assert(A && "dllExportImportClassTemplateSpecialization called "
+              "on Def without dllexport or dllimport");
+
+  // We reject explicit instantiations in class scope, so there should
+  // never be any delayed exported classes to worry about.
+  assert(S.DelayedDllExportClasses.empty() &&
+         "delayed exports present at explicit instantiation");
+  S.checkClassLevelDLLAttribute(Def);
+
+  // Propagate attribute to base class templates.
+  for (auto &B : Def->bases()) {
+    if (auto *BT = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+            B.getType()->getAsCXXRecordDecl()))
+      S.propagateDLLAttrToBaseClassTemplate(Def, A, BT, B.getLocStart());
+  }
+
+  S.referenceDLLExportedClassMethods();
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
                                  SourceLocation ExternLoc,
                                  SourceLocation TemplateLoc,
                                  unsigned TagSpec,
                                  SourceLocation KWLoc,
                                  const CXXScopeSpec &SS,
if (Old_TSK == TSK_ExplicitInstantiationDeclaration &&
            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);
-
-        // 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());
-        }
-
-        referenceDLLExportedClassMethods();
-      }
+        dllExportImportClassTemplateSpecialization(*this, Def);
+
+    // 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.
+      // 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");
+      dllExportImportClassTemplateSpecialization(*this, Def);
     }
 
     // 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 {
 

cfe/trunk/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"
 

cfe/trunk/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