[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288682: [Sema] Respect DLL attributes more faithfully 
(authored by smeenai).

Changed prior to commit:
  https://reviews.llvm.org/D26657?vs=80193=80289#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D26657

Files:
  cfe/trunk/lib/Sema/SemaTemplate.cpp
  cfe/trunk/test/CodeGenCXX/dllexport.cpp
  cfe/trunk/test/CodeGenCXX/windows-itanium-dllexport.cpp

Index: cfe/trunk/lib/Sema/SemaTemplate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp
@@ -7435,6 +7435,30 @@
   return false;
 }
 
+/// Make a dllexport or dllimport attr on a class template specialization take
+/// effect.
+static void dllExportImportClassTemplateSpecialization(
+Sema , ClassTemplateSpecializationDecl *Def) {
+  auto *A = cast_or_null(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  : Def->bases()) {
+if (auto *BT = dyn_cast_or_null(
+B.getType()->getAsCXXRecordDecl()))
+  S.propagateDLLAttrToBaseClassTemplate(Def, A, BT, B.getLocStart());
+  }
+
+  S.referenceDLLExportedClassMethods();
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
@@ -7681,24 +7705,33 @@
 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  : Def->bases()) {
-  if (auto *BT = dyn_cast_or_null(
-  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() &&
+(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);
Index: cfe/trunk/test/CodeGenCXX/dllexport.cpp
===
--- cfe/trunk/test/CodeGenCXX/dllexport.cpp
+++ cfe/trunk/test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+  // In the MS ABI, an explicit instantiation definition can add a dll

smeenai wrote:
> hans wrote:
> > smeenai wrote:
> > > hans wrote:
> > > > Why the isWindowsItaniumEnvironment check? I'd expect checking for the 
> > > > MS ABI is sufficient?
> > > 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).
> > 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.
> You're right; the current situation isn't ideal. I can clean that up in a 
> follow-up.
Sounds good.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+  // In the MS ABI, an explicit instantiation definition can add a dll

hans wrote:
> smeenai wrote:
> > hans wrote:
> > > Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS 
> > > ABI is sufficient?
> > 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).
> 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.
You're right; the current situation isn't ideal. I can clean that up in a 
follow-up.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 80193.
smeenai added a comment.

Addressing comments


https://reviews.llvm.org/D26657

Files:
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp

Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7435,6 +7435,30 @@
   return false;
 }
 
+/// Make a dllexport or dllimport attr on a class template specialization take
+/// effect.
+static void dllExportImportClassTemplateSpecialization(
+Sema , ClassTemplateSpecializationDecl *Def) {
+  auto *A = cast_or_null(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  : Def->bases()) {
+if (auto *BT = dyn_cast_or_null(
+B.getType()->getAsCXXRecordDecl()))
+  S.propagateDLLAttrToBaseClassTemplate(Def, A, BT, B.getLocStart());
+  }
+
+  S.referenceDLLExportedClassMethods();
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
@@ -7681,24 +7705,33 @@
 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  : Def->bases()) {
-  if (auto *BT = dyn_cast_or_null(
-  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() &&
+(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
+  // 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should

smeenai wrote:
> hans wrote:
> > 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.
> 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.
You're right, my mistake.



Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+  // In the MS ABI, an explicit instantiation definition can add a dll

smeenai wrote:
> hans wrote:
> > Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS 
> > ABI is sufficient?
> 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).
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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: include/clang/Sema/Sema.h:7494
 
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,

hans wrote:
> Nit: I think `///` implies `\brief`, so we don't usually include it.
Cool, will drop.



Comment at: include/clang/Sema/Sema.h:7495
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr);

hans wrote:
> hans wrote:
> > 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`
> also it should be in the `private:` section since it's just a helper. And 
> maybe the "check" part is unnecessary? `dllExportClassTemplateSpecialization` 
> might work.
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 :)



Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should

hans wrote:
> 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.
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.



Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+  // In the MS ABI, an explicit instantiation definition can add a dll

hans wrote:
> Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS ABI 
> is sufficient?
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).


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Apologies for the delay. I was out last week.

In https://reviews.llvm.org/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.




Comment at: include/clang/Sema/Sema.h:7494
 
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,

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



Comment at: include/clang/Sema/Sema.h:7495
+  /// \brief Make an existing DLL attribute on a class take effect.
+  void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr);

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`



Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr) {
+  // We reject explicit instantiations in class scope, so there should

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.



Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+  // In the MS ABI, an explicit instantiation definition can add a dll

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


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Ping.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added a comment.

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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai updated this revision to Diff 78621.
smeenai added a comment.

Typo in comment


https://reviews.llvm.org/D26657

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp

Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7435,6 +7435,23 @@
   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  : Def->bases()) {
+if (auto *BT = dyn_cast_or_null(
+B.getType()->getAsCXXRecordDecl()))
+  propagateDLLAttrToBaseClassTemplate(Def, Attr, BT, B.getLocStart());
+  }
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
@@ -7681,23 +7698,34 @@
 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  : Def->bases()) {
-  if (auto *BT = dyn_cast_or_null(
-  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() &&
+(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 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai updated this revision to Diff 78620.
smeenai updated the summary for this revision.
smeenai added a comment.

Addressing comments


https://reviews.llvm.org/D26657

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp

Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7435,6 +7435,23 @@
   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  : Def->bases()) {
+if (auto *BT = dyn_cast_or_null(
+B.getType()->getAsCXXRecordDecl()))
+  propagateDLLAttrToBaseClassTemplate(Def, Attr, BT, B.getLocStart());
+  }
+}
+
 // Explicit instantiation of a class template specialization
 DeclResult
 Sema::ActOnExplicitInstantiation(Scope *S,
@@ -7681,23 +7698,35 @@
 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  : Def->bases()) {
-  if (auto *BT = dyn_cast_or_null(
-  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() &&
+(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 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
 (TSK == TSK_ExplicitInstantiationDefinition ||
  DLLImportExplicitInstantiationDef)) {

hans wrote:
> smeenai wrote:
> > rnk wrote:
> > > 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.
> > 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.
> 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?
That's a fair point. I'll modify this to address that.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via cfe-commits]

hans added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
 (TSK == TSK_ExplicitInstantiationDefinition ||
  DLLImportExplicitInstantiationDef)) {

smeenai wrote:
> rnk wrote:
> > 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.
> 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.
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?


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai updated this revision to Diff 78324.
smeenai added a comment.

Moving explanation to comment instead of referencing Phabricator


https://reviews.llvm.org/D26657

Files:
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp


Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate 
ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 
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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7663,20 +7663,31 @@
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)) {
+// Fix a TSK_ExplicitInstantiationDeclaration or a 
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration &&
+ (TSK == TSK_ExplicitInstantiationDefinition ||
+  DLLImportExplicitInstantiationDef)) ||
+(Old_TSK == TSK_ImplicitInstantiation &&
+ TSK == TSK_ExplicitInstantiationDefinition)) {
   // FIXME: Need to notify the ASTMutationListener that we did this.
   Def->setTemplateSpecializationKind(TSK);
 
-  if (!getDLLAttr(Def) && getDLLAttr(Specialization) &&
+  if (((!getDLLAttr(Def) && getDLLAttr(Specialization)) ||
+   (Old_TSK == TSK_ImplicitInstantiation &&
+Specialization->hasAttr())) &&
   (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.
+// attribute to a template with a previous instantiation declaration
+// or implicit instantiation. MinGW doesn't allow this. In the implicit
+// instantiation case, 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.
 auto *A = cast(
 getDLLAttr(Specialization)->clone(getASTContext()));
 A->setInherited(true);


Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[David Majnemer via cfe-commits]

majnemer added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7683-7685
+// or implicit instantiation. MinGW doesn't allow this. In the implicit
+// instantiation case, we limit clang to only adding dllexport; see the
+// discussion in https://reviews.llvm.org/D26657 for details.

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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai updated this revision to Diff 78322.
smeenai added a comment.

Limiting to dllexport after discussion with @hans


https://reviews.llvm.org/D26657

Files:
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp


Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate 
ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 
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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7663,20 +7663,26 @@
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)) {
+// Fix a TSK_ExplicitInstantiationDeclaration or a 
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration &&
+ (TSK == TSK_ExplicitInstantiationDefinition ||
+  DLLImportExplicitInstantiationDef)) ||
+(Old_TSK == TSK_ImplicitInstantiation &&
+ TSK == TSK_ExplicitInstantiationDefinition)) {
   // FIXME: Need to notify the ASTMutationListener that we did this.
   Def->setTemplateSpecializationKind(TSK);
 
-  if (!getDLLAttr(Def) && getDLLAttr(Specialization) &&
+  if (((!getDLLAttr(Def) && getDLLAttr(Specialization)) ||
+   (Old_TSK == TSK_ImplicitInstantiation &&
+Specialization->hasAttr())) &&
   (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.
+// attribute to a template with a previous instantiation declaration
+// or implicit instantiation. MinGW doesn't allow this. In the implicit
+// instantiation case, we limit clang to only adding dllexport; see the
+// discussion in https://reviews.llvm.org/D26657 for details.
 auto *A = cast(
 getDLLAttr(Specialization)->clone(getASTContext()));
 A->setInherited(true);


Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // M32-DAG: define weak_odr 

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
 (TSK == TSK_ExplicitInstantiationDefinition ||
  DLLImportExplicitInstantiationDef)) {

rnk wrote:
> 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.
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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Reid Kleckner via cfe-commits]

rnk added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
 (TSK == TSK_ExplicitInstantiationDefinition ||
  DLLImportExplicitInstantiationDef)) {

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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via cfe-commits]

hans added a comment.

In https://reviews.llvm.org/D26657#597859, @smeenai wrote:

> In https://reviews.llvm.org/D26657#597523, @hans wrote:
>
> > In https://reviews.llvm.org/D26657#596897, @smeenai wrote:
> >
> > > In https://reviews.llvm.org/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 https://reviews.llvm.org/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`.




https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added a comment.

In https://reviews.llvm.org/D26657#597523, @hans wrote:

> In https://reviews.llvm.org/D26657#596897, @smeenai wrote:
>
> > In https://reviews.llvm.org/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 https://reviews.llvm.org/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`.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via cfe-commits]

hans added a comment.

In https://reviews.llvm.org/D26657#596897, @smeenai wrote:

> In https://reviews.llvm.org/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 https://reviews.llvm.org/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?


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added a comment.

In https://reviews.llvm.org/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 https://reviews.llvm.org/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.


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Hans Wennborg via cfe-commits]

hans added a comment.

> 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?


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7669
+// Fix a TSK_ExplicitInstantiationDeclaration or a 
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||

compnerd wrote:
> The fragment doesn't make sense to me.  Perhaps needs a bit more context?
Do you mean the comment specifically?


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Saleem Abdulrasool via cfe-commits]

compnerd added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:7669
+// Fix a TSK_ExplicitInstantiationDeclaration or a 
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||

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


https://reviews.llvm.org/D26657



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26657: [Sema] Respect DLL attributes more faithfully

[Shoaib Meenai via cfe-commits]

smeenai created this revision.
smeenai added reviewers: compnerd, hans.
smeenai added a subscriber: cfe-commits.

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.


https://reviews.llvm.org/D26657

Files:
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/dllexport.cpp
  test/CodeGenCXX/dllimport.cpp
  test/CodeGenCXX/windows-itanium-dllexport.cpp


Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
===
--- test/CodeGenCXX/windows-itanium-dllexport.cpp
+++ test/CodeGenCXX/windows-itanium-dllexport.cpp
@@ -23,3 +23,8 @@
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcEaSERKS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIcE1fEv
 
+c g;
+template class __declspec(dllexport) c;
+
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdEaSERKS0_
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIdE1fEv
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -814,6 +814,13 @@
 // M32-DAG: {{declare|define available_externally}} dllimport x86_thiscallcc 
void @"\01?f@?$ExplicitInstantiationDeclExportedDefImportedTemplate@H@@QAEXXZ"
 // M32-DAG: {{declare|define available_externally}} dllimport x86_thiscallcc 
%struct.ExplicitInstantiationDeclExportedDefImportedTemplate* 
@"\01??0?$ExplicitInstantiationDeclExportedDefImportedTemplate@H@@QAE@XZ"
 
+template  struct 
ImplicitInstantiationExplicitInstantiationDefImportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefImportedTemplate 
ImplicitInstantiationExplicitInstantiationDefImportedTemplateInstance;
+template class __declspec(dllimport) 
ImplicitInstantiationExplicitInstantiationDefImportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefImportedTemplate, 
f);
+// M32-DAG: declare dllimport x86_thiscallcc void 
@"\01?f@?$ImplicitInstantiationExplicitInstantiationDefImportedTemplate@H@@QAEXXZ"
+// G32-DAG: define weak_odr x86_thiscallcc void 
@_ZN61ImplicitInstantiationExplicitInstantiationDefImportedTemplateIiE1fEv
+
 template  struct PR23770BaseTemplate { void f() {} };
 template  struct PR23770DerivedTemplate : PR23770BaseTemplate 
{};
 extern template struct PR23770DerivedTemplate;
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -771,6 +771,13 @@
 // 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  struct 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate { void f() {} };
+ImplicitInstantiationExplicitInstantiationDefExportedTemplate 
ImplicitInstantiationExplicitInstantiationDefExportedTemplateInstance;
+template class __declspec(dllexport) 
ImplicitInstantiationExplicitInstantiationDefExportedTemplate;
+USEMEMFUNC(ImplicitInstantiationExplicitInstantiationDefExportedTemplate, 
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*);
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7665,20 +7665,22 @@
Specialization->getDefinition());
   if (Def) {
 TemplateSpecializationKind Old_TSK = Def->getTemplateSpecializationKind();
-// Fix a TSK_ExplicitInstantiationDeclaration followed by a
-// TSK_ExplicitInstantiationDefinition
-if (Old_TSK == TSK_ExplicitInstantiationDeclaration &&
+// Fix a TSK_ExplicitInstantiationDeclaration or a 
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
 (TSK == TSK_ExplicitInstantiationDefinition ||
  DLLImportExplicitInstantiationDef)) {
   // FIXME: Need to notify the ASTMutationListener that we did this.
   Def->setTemplateSpecializationKind(TSK);
 
-  if (!getDLLAttr(Def) && getDLLAttr(Specialization) &&
+  if (getDLLAttr(Specialization) &&
+  (!getDLLAttr(Def) || Old_TSK == TSK_ImplicitInstantiation) &&
   (Context.getTargetInfo().getCXXABI().isMicrosoft() ||