Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Blaikie via cfe-commits]

On Tue, Feb 14, 2017 at 4:21 PM Mehdi AMINI via Phabricator via cfe-commits
 wrote:

> mehdi_amini added a comment.
>
> In https://reviews.llvm.org/D13330#582607, @rsmith wrote:
>
> > I think this attribute is poorly named. Explicit instantiation
> definitions are *already* required to be globally unique; see
> [temp.spec]/5.1:
> >
> > "For a given template and a given set of template-arguments, an explicit
> instantiation definition shall appear at most once in a program"
>
>
> So what prevents from emitting these as "strong" symbols without any
> attribute? We should have the guarantee that we have only one such copy in
> the program.
>

If I recall correctly - the requirement is that if there's an explicit
instantiation declaration there must be an explicit instantiation
definition, but it's not required that all sites required an instantiation
should see/have an explicit instantiation declaration. So you might have
some implicit instantiations scattered around, then one explicit
instantiation. If the explicit instantiation were strong, you'd get
duplicate symbol problems in your link.

(that's my naive/vague understanding anyway)


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D13330
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D13330#582607, @rsmith wrote:

> I think this attribute is poorly named. Explicit instantiation definitions 
> are *already* required to be globally unique; see [temp.spec]/5.1:
>
> "For a given template and a given set of template-arguments, an explicit 
> instantiation definition shall appear at most once in a program"


So what prevents from emitting these as "strong" symbols without any attribute? 
We should have the guarantee that we have only one such copy in the program.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330



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

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro planned changes to this revision.
loladiro added a comment.

Thanks for the review! Sorry, it's been a while since I wrote this code, so I'm 
not fluent in all the details, but I've replied below. I am not particularly 
partial to the name, so whatever you feel is best is ok with me.




Comment at: include/clang/Basic/AttrDocs.td:2321-2323
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.

rsmith wrote:
> The primary description of the attribute should specify what it means, not 
> the consequences of that meaning. In this case, the meaning is that the 
> programmer is guaranteeing to the compiler that an explicit instantiation 
> precedes all implicit instantiations, and the consequence is that we can use 
> strong symbols.
Ok, sounds good.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
+def err_unique_instantiation_not_previous : Error<
+  "'unique_instantiation' attribute must be specified for all declarations and 
definitions of this explicit template instantiation">;
 

rsmith wrote:
> loladiro wrote:
> > aaron.ballman wrote:
> > > > They are checking for two different conditions in the spec. One 
> > > > requires that all explicit template instantiations with this attribute 
> > > > have a declaration, the other that every declaration/definition has the 
> > > > attribute.
> > > 
> > > Okay, I see now what this diagnostic is attempting to convey. I think it 
> > > should read:
> > > ```
> > > def err_unique_instantiation_no_declaration : Error<
> > >   "'unique_instantiation' attribute on an explicit instantiation requires 
> > > a previous explicit instantiation declaration">;
> > > ```
> > Sounds good.
> This seems like an unnecessarily-strict rule to me. I don't see a reason to 
> disallow:
> ```
> // in header
> extern template struct __attribute__((whatever_its_called)) X;
> ```
> ```
> // in source file
> template struct X;
> ```
> There doesn't appear to be any safety or correctness concern here. In 
> general, it seems like the attribute is only promising that the explicit 
> instantiation declaration precedes any other instantiation, so the only 
> necessary rule would seem to be: if the attribute is present on any explicit 
> instantiation, the first point of instantiation must be an explicit 
> instantiation declaration that has the attribute.
While I didn't write the rules (just implemented them from the original 
proposal), I like the requirement from the point of view of making sure that 
the explicit instantiation declaration doesn't accidentally get deleted. Those 
declarations are somewhat odd in that they can be deleted without any semantic 
loss in functionality (just larger code size/more work for the linker) in the 
absence of this attribute. However, with this attribute, deleting the 
declaration would cause a subtle, perhaps hard to track down semantic change, 
so I like requiring people to be explicit. 



Comment at: lib/Sema/SemaDecl.cpp:2398-2399
+  // template declarations. In clang getDefinition() will get the
+  // ClassTemplateSpecializationDecl associated with the class template
+  // declaration, so we'd give incorrect warnings here.
+  if (auto *CTSD = dyn_cast(New)) {

rsmith wrote:
> I have no idea what you mean by this. Did you mean "associated with the 
> explicit instantiation declaration" or something like that?
Yes, I believe that is what I meant. It's been a while since I wrote this code, 
but I believe that all I'm trying to say here is that explicit instantiation 
declarations/definitions are allowed to add attributes after a definition, even 
though that is not normally allowed.



Comment at: lib/Sema/SemaDeclAttr.cpp:5400
+  if (!CTSD->getPreviousDecl())
+S.Diag(Attr.getLoc(), diag::err_unique_instantiation_no_declaration);
+}

rsmith wrote:
> Why is this rule only applied to explicit instantiations of class templates, 
> and not to function or variable templates?
I don't remember precisely, but I believe (at least when I wrote the patch) 
that those prior explicit instantiation declarations did not leave an AST node 
that I could check for.



Comment at: lib/Sema/SemaDeclCXX.cpp:5666-5685
+/// \brief Check if the new class needs a unique instantiation attribute
+/// based on whether its containing function or class has it
+void Sema::checkClassLevelUniqueInstantiation(CXXRecordDecl *Record) {
+  Decl *D = Record;
+  if (!D || D->hasAttr())
+return;
+  while (DeclContext *DC = D ? D->getDeclContext() : nullptr) {

rsmith wrote:
> I don't think this is the right way to handle this case. Note that an 
> explicit instantiation declaration/definition for a class is only an explicit 
> instantiation of those m

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Richard Smith via cfe-commits]

rsmith added a comment.

I think this attribute is poorly named. Explicit instantiation definitions are 
*already* required to be globally unique; see [temp.spec]/5.1:

"For a given template and a given set of template-arguments, an explicit 
instantiation definition shall appear at most once in a program"

The actual meaning of the attribute is that an explicit instantiation 
declaration will appear before any use that might trigger implicit 
instantiation. To that end, something like 
`__attribute__((declared_before_all_uses))` might be clearer.




Comment at: include/clang/Basic/AttrDocs.td:2321-2323
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.

The primary description of the attribute should specify what it means, not the 
consequences of that meaning. In this case, the meaning is that the programmer 
is guaranteeing to the compiler that an explicit instantiation precedes all 
implicit instantiations, and the consequence is that we can use strong symbols.



Comment at: include/clang/Basic/AttrDocs.td:1736-1755
+// Explicit template definition (in exactly ONE .cpp file)
+template struct __attribute__((unique_instantiation)) my_template;
+
+
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.

loladiro wrote:
> majnemer wrote:
> > Instead of using all-caps for emphasis in "ONE" and "MUST", why not bold 
> > the text instead?  It would catch the eye a little faster.
> Sure! I assume, this follows standard RST conventions where ** is bold?
Yes, this documentation allows arbitrary RST markup. But I don't see the need 
for strong emphasis here.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
+def err_unique_instantiation_not_previous : Error<
+  "'unique_instantiation' attribute must be specified for all declarations and 
definitions of this explicit template instantiation">;
 

loladiro wrote:
> aaron.ballman wrote:
> > > They are checking for two different conditions in the spec. One requires 
> > > that all explicit template instantiations with this attribute have a 
> > > declaration, the other that every declaration/definition has the 
> > > attribute.
> > 
> > Okay, I see now what this diagnostic is attempting to convey. I think it 
> > should read:
> > ```
> > def err_unique_instantiation_no_declaration : Error<
> >   "'unique_instantiation' attribute on an explicit instantiation requires a 
> > previous explicit instantiation declaration">;
> > ```
> Sounds good.
This seems like an unnecessarily-strict rule to me. I don't see a reason to 
disallow:
```
// in header
extern template struct __attribute__((whatever_its_called)) X;
```
```
// in source file
template struct X;
```
There doesn't appear to be any safety or correctness concern here. In general, 
it seems like the attribute is only promising that the explicit instantiation 
declaration precedes any other instantiation, so the only necessary rule would 
seem to be: if the attribute is present on any explicit instantiation, the 
first point of instantiation must be an explicit instantiation declaration that 
has the attribute.



Comment at: lib/AST/ASTContext.cpp:8789
 
-  return GVA_DiscardableODR;
+  return  GVA_DiscardableODR;
 }

revert this change



Comment at: lib/Sema/SemaDecl.cpp:2396-2397
+  // Explicit template instantiations need special handling because in certain
+  // ABIs explicit template definitions may add attributes over explicit
+  // template declarations. In clang getDefinition() will get the
+  // ClassTemplateSpecializationDecl associated with the class template

I assume you mean "explicit instantiation" rather than "explicit template" here 
(2x).



Comment at: lib/Sema/SemaDecl.cpp:2398-2399
+  // template declarations. In clang getDefinition() will get the
+  // ClassTemplateSpecializationDecl associated with the class template
+  // declaration, so we'd give incorrect warnings here.
+  if (auto *CTSD = dyn_cast(New)) {

I have no idea what you mean by this. Did you mean "associated with the 
explicit instantiation declaration" or something like that?



Comment at: lib/Sema/SemaDeclAttr.cpp:5400
+  if (!CTSD->getPreviousDecl())
+S.Diag(Attr.getLoc(), diag::err_unique_instantiation_no_declaration);
+}

Why is this rule only applied to explicit instantiations of class templates, 
and not to function or variable templates?



Comment at: lib/Sema/SemaDeclCXX.cpp:5666-5685
+/// \brief Check if the new class needs a unique instantiation attribute
+/

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 76256.
loladiro added a comment.

Fix for the corner case I found and add it as a test.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2662,6 +2662,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar {};// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {}// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct __attribute__((unique_instantiation(16))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
+
+template 
+struct static_separate_template {
+typedef T element;
+static T *a_static_field;
+};
+extern template struct __attribute__((unique_instantiation)) static_separate_template;
+template struct __attribute__((unique_instantiation)) static_separate_template;
+extern template struct __attribute__((unique_instantiation)) static_separate_template; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) static_separate_template;
+
+template  typename static_se

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

I meant 
`template __attribute__((unique_instantiation)) int * 
static_separate_template::a_static_field;`
of course, though we probably need a better diagnostic for the other spelling 
(which applies the attribute to the static_separate_template). I'll look 
into adding a diagnostic.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330



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

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

I found a bug in this which I need to fix, but while I'm at it, in doing more 
testing on this, I came across the following corner case:

  template 
  struct static_separate_template {
  typedef T element;
  static T *a_static_field;
  };
  extern template struct __attribute__((unique_instantiation)) 
static_separate_template;
  template struct __attribute__((unique_instantiation)) 
static_separate_template;
  extern template struct __attribute__((unique_instantiation)) 
static_separate_template;
  template struct __attribute__((unique_instantiation)) 
static_separate_template;
  
  template  typename static_separate_template::element 
*static_separate_template::a_static_field = nullptr;
  template int * __attribute__((unique_instantiation)) 
static_separate_template::a_static_field;
  template char * static_separate_template::a_static_field; // 
expected-error{{'unique_instantiation' attribute must be specified for all 
declarations and definitions of this explicit template instantiation}}

How should this be handled? I indicated my inclination in the comment, but I'm 
open to alternative suggestions.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330



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

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro set the repository for this revision to rL LLVM.
loladiro updated this revision to Diff 76102.
loladiro added a comment.

Rebased on current master.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2662,6 +2662,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar {};// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {}// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct __attribute__((unique_instantiation(16))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) f

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Majnemer via cfe-commits]

majnemer added a comment.

I think this looks good but I'd like @rsmith to take a look.


https://reviews.llvm.org/D13330



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

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

I came across a situation again where this would be useful to have. I know this 
was approved, but looking it looks like I wanted @majnemer to have another look.


https://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

Bumping this again.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 43218.
loladiro added a comment.

Rebased


http://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2369,6 +2369,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar {};// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {}// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct __attribute__((unique_instantiation(16))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

@majnemer Do you like the new approach? Is there anything else to be done here?


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 42137.
loladiro added a comment.

Propagate unique instantiation attribute to children rather than later trying 
to look through the DeclContexts to see if we're contained in one that has the 
attribute.


http://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2373,6 +2373,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar {};// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {}// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct __attribute__((unique_instantiation(16))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 42132.
loladiro added a comment.

Address David's concern about inner classes. David also suggested on IRC to 
propagate
the unique instantiation attribute down rather than walking the context chain to
check for the attribute. I'll try that out, but wanted to put up a known-good 
version
with all fixed first.


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2373,6 +2373,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template  T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar{}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {} // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template  T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1;  // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct  __attribute__((unique_instantiation(16))) foo6;// expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,69 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template  T pi = T(3.1415926535897932385);
+extern template __attribute_

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: lib/AST/ASTContext.cpp:8257-8266
@@ -8256,1 +8256,12 @@
 
+bool ASTContext::containedInUniqueInstantiation(const Decl *D) {
+  const RecordDecl *RD;
+  while ((RD = dyn_cast(D->getDeclContext( {
+auto *CTSD = dyn_cast(RD);
+if (CTSD && CTSD->hasAttr())
+  return true;
+D = RD;
+  }
+  return false;
+}
+

majnemer wrote:
> Function templates can contain class templates which contain functions.  I 
> think this code will return false if the outermost function template is the 
> only one annotated with UniqueInstantiationAttr.
So you're concerned about this case:
```
template < typename T > auto foo( T x ) {
  class inner {
  T y;
  public:
  virtual ~inner() {}
  inner(T y) : y(y) {}
  T getY() { return y; }
  };
  return inner{x}.getY();
}

extern template __attribute__((unique_instantiation)) auto foo(int);
template __attribute__((unique_instantiation)) auto foo(int);
```
Right now the inner class's vtable and method is linkonce_odr. If you think it 
should be strong I'll look into it.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Majnemer via cfe-commits]

majnemer added inline comments.


Comment at: lib/AST/ASTContext.cpp:8257-8266
@@ -8256,1 +8256,12 @@
 
+bool ASTContext::containedInUniqueInstantiation(const Decl *D) {
+  const RecordDecl *RD;
+  while ((RD = dyn_cast(D->getDeclContext( {
+auto *CTSD = dyn_cast(RD);
+if (CTSD && CTSD->hasAttr())
+  return true;
+D = RD;
+  }
+  return false;
+}
+

Function templates can contain class templates which contain functions.  I 
think this code will return false if the outermost function template is the 
only one annotated with UniqueInstantiationAttr.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 42100.
loladiro added a comment.

Address stylistic concerns brought up by David


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2373,6 +2373,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template  T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar{}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {} // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template  T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1;  // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct  __attribute__((unique_instantiation(16))) foo6;// expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template  T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+bar(T y) : y(y) {}
+  };
+};
+template 
+T b

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/AttrDocs.td:1736-1755
@@ -1722,2 +1735,22 @@
+
+// Explicit template definition (in exactly ONE .cpp file)
+template struct __attribute__((unique_instantiation)) my_template;
+
+
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.
+
+If the attribute is present on one such definition or declaration for a given
+entity, it must be present on all.
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In
+particular this means that any usage of the entity has to be preceeded by an
+appropriate explicit template declaration or definition.
 
+It is thus recommended that explicit template declarations are placed in 
headers
+to suppress any potential implicit instantiation of the entity. In order to
+encourage this programming style, any explicit template definition with this
+attribute MUST be preceeded by an appropriate declaration.
   }];

majnemer wrote:
> Instead of using all-caps for emphasis in "ONE" and "MUST", why not bold the 
> text instead?  It would catch the eye a little faster.
Sure! I assume, this follows standard RST conventions where ** is bold?


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2533-2534
@@ -2531,1 +2532,4 @@
   ":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+  "'unique_instantiation' attribute only applies to an explicit template 
declaration or instantiation">;
+def err_unique_instantiation_no_declaration : Error<

majnemer wrote:
> I can't seem to see any users of `err_unique_instantiation_wrong_decl`.
Yes, it was replaced by adding a case to err_attribute_wrong_decl_type above. 
Will remove.


Comment at: lib/AST/ASTContext.cpp:8283-8286
@@ +8282,6 @@
+Context.containedInUniqueInstantiation(FD)) {
+  // We return GVA_StrongExternal here, instead of going through the logic
+  // below, because even if the definition is available inline, since the
+  // source specified  an explicit template instantiation, we want to make
+  // the symbol available.
+  return GVA_StrongExternal;

majnemer wrote:
> There are a lot of commas here, making it a little hard to read the 
> justification.
> 
> Perhaps something along the lines of:
> 
> > This translation unit is responsible for emitting a unique instantiation of 
> > this function regardless of whether or not the function is marked inline.
Sounds good.


Comment at: lib/CodeGen/CGVTables.cpp:744-747
@@ -743,2 +743,6 @@
   case TSK_ExplicitInstantiationDefinition:
+// If the key function has strong linkage (say due to
+// UniqueInstantiationAttr), the VTable should too.
+if (Context.containedInUniqueInstantiation(keyFunction))
+  return llvm::GlobalVariable::ExternalLinkage;
 return !Context.getLangOpts().AppleKext ?

majnemer wrote:
> What if the key function is not contained within a record which is marked as 
> `UniqueInstantiationAttr` but the key function itself is marked 
> `UniqueInstantiationAttr`?
I don't know, I'd be inclined to say the unique instantiation does not apply to 
the vtable unless it's explicit on the class, but I'd be open to differing 
opinions.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Majnemer via cfe-commits]

majnemer added inline comments.


Comment at: include/clang/Basic/AttrDocs.td:1736-1755
@@ -1722,2 +1735,22 @@
+
+// Explicit template definition (in exactly ONE .cpp file)
+template struct __attribute__((unique_instantiation)) my_template;
+
+
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.
+
+If the attribute is present on one such definition or declaration for a given
+entity, it must be present on all.
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In
+particular this means that any usage of the entity has to be preceeded by an
+appropriate explicit template declaration or definition.
 
+It is thus recommended that explicit template declarations are placed in 
headers
+to suppress any potential implicit instantiation of the entity. In order to
+encourage this programming style, any explicit template definition with this
+attribute MUST be preceeded by an appropriate declaration.
   }];

Instead of using all-caps for emphasis in "ONE" and "MUST", why not bold the 
text instead?  It would catch the eye a little faster.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2533-2534
@@ -2531,1 +2532,4 @@
   ":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+  "'unique_instantiation' attribute only applies to an explicit template 
declaration or instantiation">;
+def err_unique_instantiation_no_declaration : Error<

I can't seem to see any users of `err_unique_instantiation_wrong_decl`.


Comment at: lib/AST/ASTContext.cpp:8283-8286
@@ +8282,6 @@
+Context.containedInUniqueInstantiation(FD)) {
+  // We return GVA_StrongExternal here, instead of going through the logic
+  // below, because even if the definition is available inline, since the
+  // source specified  an explicit template instantiation, we want to make
+  // the symbol available.
+  return GVA_StrongExternal;

There are a lot of commas here, making it a little hard to read the 
justification.

Perhaps something along the lines of:

> This translation unit is responsible for emitting a unique instantiation of 
> this function regardless of whether or not the function is marked inline.


Comment at: lib/CodeGen/CGVTables.cpp:744-747
@@ -743,2 +743,6 @@
   case TSK_ExplicitInstantiationDefinition:
+// If the key function has strong linkage (say due to
+// UniqueInstantiationAttr), the VTable should too.
+if (Context.containedInUniqueInstantiation(keyFunction))
+  return llvm::GlobalVariable::ExternalLinkage;
 return !Context.getLangOpts().AppleKext ?

What if the key function is not contained within a record which is marked as 
`UniqueInstantiationAttr` but the key function itself is marked 
`UniqueInstantiationAttr`?


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

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

LGTM, but please wait for David to sign off before committing.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

Bump, is there anything else that's needed here?


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 41265.
loladiro added a comment.

Add support for variable templates


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2371,6 +2371,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang -cc1 -std=c++14 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+// Usages on non-templates
+float __attribute__((unique_instantiation)) notpi(2.71828182845904523536028747135); // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+struct __attribute__((unique_instantiation)) bar {};// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+void __attribute__((unique_instantiation)) func() {}// expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+// Usages that violate one of the conditions required conditions
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+T pi1 = T(3.1415926535897932385);
+template __attribute__((unique_instantiation)) float pi1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct __attribute__((unique_instantiation(16))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang -std=c++14 -emit-llvm -c -S -o - %s | FileCheck %s
+
+// CHECK: @_Z2piIfE = global float
+// CHECK-NOT: @_Z2piIfE = weak_odr global float
+template 
+T pi = T(3.1415926535897932385);
+extern template __attribute__((unique_instantiation)) float pi;
+template __attribute__((unique_instantiation)) float pi;
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 41261.
loladiro added a comment.

Rebased and made the small suggested changes to the test cases.


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2371,6 +2371,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+// Correct usage
+template 
+struct foo {};
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+// Various examples of incorrect usage
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct  __attribute__((unique_instantiation(16))) foo6;// expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang -std=c++11 -emit-llvm -c -S -o - %s | FileCheck %s
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+bar(T y) : y(y) {}
+  };
+};
+template 
+T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template 
+T bar() {
+  return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+  auto var = foo{5};
+  auto var2 = foo::bar{5};
+  auto x = bar();
+  return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7398,20 +7398,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add 

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Modulo the question you and David are discussing about variable templates (for 
which I don't have an answer handy), I just have a few small testing nits.



Comment at: test/SemaCXX/unique-instantiations.cpp:24
@@ +23,3 @@
+template struct foo5;  // 
expected-error{{'unique_instantiation' attribute must be specified for all 
declarations and definitions of this explicit template instantiation}}
+
+template 

> Isn't the correct usage checked for in the CodeGen tests above?

It is, but those are just tests for code generation, which are all expected to 
succeed semantically. We generally expect tests in Sema (and related folders) 
that demonstrate successful cases as well as failure cases for features.


Comment at: test/SemaCXX/unique-instantiations.cpp:28
@@ +27,2 @@
+extern template struct  __attribute__((unique_instantiation(16))) foo6;   
 // expected-error{{'unique_instantiation' attribute takes no arguments}}
+template struct __attribute__((unique_instantiation("Hello World"))) 
foo6; // expected-error{{'unique_instantiation' attribute takes no 
arguments}}

The test with unique_instantiation(16) is sufficient, no need for the "Hello 
World" test as well.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 39627.
loladiro updated the summary for this revision.
loladiro added a comment.

Address review feedback regarding diagnostic wording/expand tests to full text 
of diagnostic.


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2371,6 +2371,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{'unique_instantiation' attribute only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation}}
+
+template 
+struct foo6 {};
+extern template struct  __attribute__((unique_instantiation(16))) foo6;// expected-error{{'unique_instantiation' attribute takes no arguments}}
+template struct __attribute__((unique_instantiation("Hello World"))) foo6; // expected-error{{'unique_instantiation' attribute takes no arguments}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang -std=c++11 -emit-llvm -c -S -o - %s | FileCheck %s
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+bar(T y) : y(y) {}
+  };
+};
+template 
+T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template 
+T bar() {
+  return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+  auto var = foo{5};
+  auto var2 = foo::bar{5};
+  auto x = bar();
+  return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7330,20 +7330,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl)

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];

loladiro wrote:
> majnemer wrote:
> > They work ok, clang just thinks that it's a declaration of a variable 
> > template.  Try this:
> >   template  T n = T();
> >   extern template int n;
> >   template int n;
> I see. I'll look into adding support for it. Can you explain why my example 
> doesn't work? GCC seems to allow this.
Bump on the question of differences to GCC here.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "'unique_instantiation' attribute must be specified for all declarations and 
definitions of this explicit template instantiation">;
 

aaron.ballman wrote:
> > They are checking for two different conditions in the spec. One requires 
> > that all explicit template instantiations with this attribute have a 
> > declaration, the other that every declaration/definition has the attribute.
> 
> Okay, I see now what this diagnostic is attempting to convey. I think it 
> should read:
> ```
> def err_unique_instantiation_no_declaration : Error<
>   "'unique_instantiation' attribute on an explicit instantiation requires a 
> previous explicit instantiation declaration">;
> ```
Sounds good.


Comment at: test/SemaCXX/unique-instantiations.cpp:23
@@ +22,2 @@
+extern template struct __attribute__((unique_instantiation)) foo5; // 
expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // 
expected-error{{must be specified for all declarations}}

aaron.ballman wrote:
> Missing tests for correct usage of the attribute. Missing tests of the 
> attribute diagnosing when given arguments.
Isn't the correct usage checked for in the CodeGen tests above?


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "'unique_instantiation' attribute must be specified for all declarations and 
definitions of this explicit template instantiation">;
 

> They are checking for two different conditions in the spec. One requires that 
> all explicit template instantiations with this attribute have a declaration, 
> the other that every declaration/definition has the attribute.

Okay, I see now what this diagnostic is attempting to convey. I think it should 
read:
```
def err_unique_instantiation_no_declaration : Error<
  "'unique_instantiation' attribute on an explicit instantiation requires a 
previous explicit instantiation declaration">;
```


Comment at: test/SemaCXX/unique-instantiations.cpp:5
@@ +4,3 @@
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // 
expected-error{{requires a previous declaration}}
+

Please spell the entire diagnostic out in expected-error (applies to entire 
file).


Comment at: test/SemaCXX/unique-instantiations.cpp:23
@@ +22,2 @@
+extern template struct __attribute__((unique_instantiation)) foo5; // 
expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // 
expected-error{{must be specified for all declarations}}

Missing tests for correct usage of the attribute. Missing tests of the 
attribute diagnosing when given arguments.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];

majnemer wrote:
> They work ok, clang just thinks that it's a declaration of a variable 
> template.  Try this:
>   template  T n = T();
>   extern template int n;
>   template int n;
I see. I'll look into adding support for it. Can you explain why my example 
doesn't work? GCC seems to allow this.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Majnemer via cfe-commits]

majnemer added inline comments.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];

They work ok, clang just thinks that it's a declaration of a variable template. 
 Try this:
  template  T n = T();
  extern template int n;
  template int n;


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro set the repository for this revision to rL LLVM.
loladiro updated this revision to Diff 38039.
loladiro added a comment.

Address review comments.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2339,6 +2339,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified for all declarations}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{must be specified for all declarations}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{must be specified for all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang -std=c++11 -emit-llvm -c -S -o - %s | FileCheck %s
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+bar(T y) : y(y) {}
+  };
+};
+template 
+T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template 
+T bar() {
+  return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+  auto var = foo{5};
+  auto var2 = foo::bar{5};
+  auto x = bar();
+  return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7330,20 +7330,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7397,14 +7399,7 @@
   }
 }
 
-// Set the template specialization kind. Make sure it is set before
-// instantiating the members which will trigger ASTConsumer callbacks.
-Specialization->setTe

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];

majnemer wrote:
> What about variable templates?
I hadn't tried before, but looking at this, I'd say they're broken in clang 
anyway and I'm gonna say fixing that is outside the scope of this revision:
```
$ cat test.cpp
template  T n;
extern template int n;
template int n;

int main() {
return n;
}
$ ./usr/bin/clang++ -std=c++14 -c test.cpp
$ nm test.o
 T main
U _Z1nIiE
$ nm test.o.gcc6
[snip]
060098c u _Z1nIiE
```


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "The unique_instantiation must be specified on all declarations and 
definitions of a particular explicit template instantiation.">;
 

aaron.ballman wrote:
> Should say: the 'unique_instantiation' attribute.
> 
> Also, I'm not keen on "a particular" as it's very non-specific.
> 
> I wonder if we need err_unique_instantiation_no_declaration and 
> err_unique_instantiation_not_previous to be separate diagnostics? Aren't they 
> both effectively saying, 
> 
> "a 'unique_instantiation' attribute must be specified for all declarations 
> and definitions of the explicit template instantiation"
They are checking for two different conditions in the spec. One requires that 
all explicit template instantiations with this attribute have a declaration, 
the other that every declaration/definition has the attribute.


Comment at: test/CodeGenCXX/unique-instantiation.cpp:1
@@ +1,2 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+

majnemer wrote:
> Is -O0 needed here?
No, removing.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[David Majnemer via cfe-commits]

majnemer added a subscriber: majnemer.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];

What about variable templates?


Comment at: lib/Sema/SemaDecl.cpp:2288
@@ +2287,3 @@
+  if (auto *CTSD = dyn_cast(New)) {
+TemplateSpecializationKind kind = CTSD->getSpecializationKind();
+if (kind == TSK_ExplicitInstantiationDeclaration ||

Capital letters for variable names.


Comment at: test/CodeGenCXX/unique-instantiation.cpp:1
@@ +1,2 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+

Is -O0 needed here?


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/AttrDocs.td:1638
@@ +1637,3 @@
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In

> I don't think there is anything that can be really done here.

That's unfortunate, but I think you may be right. Blech.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2452
@@ +2451,3 @@
+def err_unique_instantiation_wrong_decl : Error<
+  "unique_instantiation attribute on something that is not a explicit template 
declaration or instantiation.">;
+def err_unique_instantiation_no_declaration : Error<

Please quote 'unique_instantiation' in the diagnostic (here and in the 
subsequent uses).

Instead of "something", let's use "a declaration"?

"a explicit template" should be "an explicit template".

Also, diagnostics are not complete sentences, so should not be capitalized or 
end with a full stop.

Actually, how about this for a new wording:

"'unique_instantiation' attribute only applies to an explicit template 
declaration or instantiation"


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "The unique_instantiation must be specified on all declarations and 
definitions of a particular explicit template instantiation.">;
 

Should say: the 'unique_instantiation' attribute.

Also, I'm not keen on "a particular" as it's very non-specific.

I wonder if we need err_unique_instantiation_no_declaration and 
err_unique_instantiation_not_previous to be separate diagnostics? Aren't they 
both effectively saying, 

"a 'unique_instantiation' attribute must be specified for all declarations and 
definitions of the explicit template instantiation"


Comment at: lib/AST/ASTContext.cpp:8246
@@ +8245,3 @@
+if (auto *CTSD = dyn_cast(RD)) {
+  if (CTSD->hasAttr()) {
+return true;

Elide braces


Comment at: lib/Sema/SemaDecl.cpp:2184
@@ +2183,3 @@
+
+  Attr *NewAttr = New->getAttr();
+  SourceLocation NewLoc = NewAttr ? NewAttr->getLocation() : 
New->getLocStart();

hasAttr<> followed by getAttr<> that can be simplified.


Comment at: lib/Sema/SemaDecl.cpp:2287
@@ +2286,3 @@
+  // declaration, so we'd give incorrect warnings here.
+  if (auto *CTSD = dyn_cast(New)) {
+TemplateSpecializationKind kind = CTSD->getSpecializationKind();

> As far as I know, this is only called from mergeDeclAttributes, which didn't 
> use to be called on these before this patch. I also don't think any 
> attributes but dllexport or unique_instantiation apply to these declarations 
> and those two both need special processing.

Okay, thanks!


Comment at: lib/Sema/SemaTemplate.cpp:7874
@@ +7873,3 @@
+   diag::err_unique_instantiation_no_declaration);
+else if (HadDeclaration && !OldUniqueInstantiation) {
+  Diag(NewUniqueInstantiation->getLocation(),

Elide braces


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 37555.
loladiro added a comment.

Address review comments and clang-format.


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2339,6 +2339,8 @@
 case Func | ObjCMethod | Param: return "ExpectedFunctionMethodOrParameter";
 case Func | ObjCMethod: return "ExpectedFunctionOrMethod";
 case Func | Var: return "ExpectedVariableOrFunction";
+case Func | Class:
+  return "ExpectedFunctionOrClass";
 
 // If not compiling for C++, the class portion does not apply.
 case Func | Var | Class:
Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template 
+struct foo1 {};
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template 
+struct foo2 {};
+extern template struct foo2;// expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified on all declarations}}
+
+template 
+struct foo3 {};
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3;   // expected-error{{must be specified on all declarations}}
+
+template 
+struct __attribute__((unique_instantiation)) foo4 {}; // expected-error{{only applies to explicit template declarations or definitions}}
+
+template 
+struct foo5 {};
+extern template struct __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5;  // expected-error{{must be specified on all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
+template 
+struct foo {
+  T x;
+  T getX() { return x; }
+  struct bar {
+T y;
+bar(T y) : y(y) {}
+  };
+};
+template 
+T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template 
+T bar() {
+  return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+  auto var = foo{5};
+  auto var2 = foo::bar{5};
+  auto x = bar();
+  return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7330,20 +7330,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7397,14 +7399,7 @@
   }
 }
 
-// Set the template specialization kind. Make sure it is set before
-// instantiating the members which will trigger ASTConsumer callbacks.
-Specialization->setTemplateSpecializationKind(TSK);
 InstantiateClassTemplateSpe

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/AttrDocs.td:1638
@@ +1637,3 @@
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In

aaron.ballman wrote:
> In the unlikely event the user gets this wrong (lol), is there a way to 
> diagnose this on the backend? The wording here makes it sound like it's 
> possible for this to cause silent miscompiles, which would be unfortunate.
I don't think there is anything that can be really done here. I believe the 
static linker will link this just fine (there'll be one weak and one strong 
symbol). The difficulty of course arises with dynamic linkers that won't bother 
looking for any weak symbols to merge with (at least the OS X one doesn't). I 
suppose it's not really that different from missing a `weak` attribute on some 
declarations but not others.


Comment at: lib/CodeGen/CGVTables.cpp:744
@@ +743,3 @@
+// UniqueInstantiationAttr), the VTable should too.
+keyFunction->dump();
+if (Context.containedInUniqueInstantiation(keyFunction))

aaron.ballman wrote:
> Looks like some debugging code accidentally made it in.
Thanks!


Comment at: lib/Sema/SemaDecl.cpp:2285
@@ +2284,3 @@
+  // here.
+  if (auto *CTSD = dyn_cast(New)) {
+TemplateSpecializationKind kind = CTSD->getSpecializationKind();

aaron.ballman wrote:
> Isn't this a bit broad? I do agree that we don't want to give incorrect 
> warnings, but it seems like this may inadvertently silence correct warnings 
> as well. If my gut feeling is wrong (which it might be), it would be good to 
> have some test cases confirming that this doesn't silence correct 
> diagnostics. 
As far as I know, this is only called from mergeDeclAttributes, which didn't 
use to be called on these before this patch. I also don't think any attributes 
but dllexport or unique_instantiation apply to these declarations and those two 
both need special processing.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag, 
"ExpectedFunctionOrClass">;
+  let Documentation = [UniqueInstantiationDocs];

This one should be handled in ClangAttrEmitter.cpp; I can see it being used far 
more frequently than ExpectedExplicitInstantiation, and all the machinery 
should already be in place to handle it (in CalculateDiagnostic()).


Comment at: include/clang/Basic/AttrDocs.td:1638
@@ +1637,3 @@
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In

In the unlikely event the user gets this wrong (lol), is there a way to 
diagnose this on the backend? The wording here makes it sound like it's 
possible for this to cause silent miscompiles, which would be unfortunate.


Comment at: lib/CodeGen/CGVTables.cpp:744
@@ +743,3 @@
+// UniqueInstantiationAttr), the VTable should too.
+keyFunction->dump();
+if (Context.containedInUniqueInstantiation(keyFunction))

Looks like some debugging code accidentally made it in.


Comment at: lib/Sema/SemaDecl.cpp:2177
@@ -2176,1 +2176,3 @@
 
+static void checkUniqueInstantiationAttrs(Sema &S, const Decl *New, const Decl 
*Old) {
+  // Check that any previous definitions also had this attribute set.

This function has some formatting issues. Also, I feel like some of the code 
duplication could be removed.
```
if (old == new)
  return;

Loc = Old->hasAttr ? new->getLocStart() : new->getAttr->getLocation();
S.Diag(Loc, diag::blah);
S.Diag(Old->getLocStart(0, blah);
```


Comment at: lib/Sema/SemaDecl.cpp:2285
@@ +2284,3 @@
+  // here.
+  if (auto *CTSD = dyn_cast(New)) {
+TemplateSpecializationKind kind = CTSD->getSpecializationKind();

Isn't this a bit broad? I do agree that we don't want to give incorrect 
warnings, but it seems like this may inadvertently silence correct warnings as 
well. If my gut feeling is wrong (which it might be), it would be good to have 
some test cases confirming that this doesn't silence correct diagnostics. 


Comment at: lib/Sema/SemaDeclAttr.cpp:4568
@@ +4567,3 @@
+  }
+  S.Diag(Attr.getLoc(),
+ diag::err_attribute_wrong_decl_type)

The formatting of this line seems a bit weird -- did clang-format do this?


Comment at: lib/Sema/SemaTemplate.cpp:7855
@@ +7854,3 @@
+TSK_ExplicitInstantiationDeclaration;
+  bool HadUniqueInstantiation = 
Specialization->hasAttr();
+  SourceLocation OldUniqueInstantiationLoc;

Instead of hasAttr<> followed by getAttr<>, better to just call getAttr<> and 
handle null.


Comment at: lib/Sema/SemaTemplate.cpp:7868
@@ -7862,1 +7867,3 @@
 
+  if (Specialization->hasAttr()) {
+if (TSK == TSK_ExplicitInstantiationDefinition && !HadDeclaration)

Same comment here.


Comment at: lib/Sema/SemaTemplate.cpp:7877
@@ +7876,3 @@
+  } else if (HadUniqueInstantiation) {
+Diag(D.getIdentifierLoc(),diag::err_unique_instantiation_not_previous);
+Diag(OldUniqueInstantiationLoc,diag::note_previous_explicit_instantiation);

Formatting; I would recommend running clang-format over the patch to catch 
these sort of things.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

Ok, I have tested this more extensively now. I'm happy with the results. Here's 
a patch that applies this to LLVM for example: 
https://gist.github.com/Keno/79b08a4b187c4d950dd0

Before:

  $llvm-objdump -weak-bind libLLVM-3.8svn.dylib | wc -l
   300

After:

  $llvm-objdump -weak-bind libLLVM-3.8svn.dylib | wc -l
   15

with none of those being LLVM symbols.


http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 36476.
loladiro added a comment.

Also set the correct linkage on vtables of classes with the new attribute.


http://reviews.llvm.org/D13330

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGVTables.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp

Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template < typename T > struct foo1 { };
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template < typename T > struct foo2 { };
+extern template struct foo2; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct foo3 { };
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct __attribute__((unique_instantiation)) foo4 { }; // expected-error{{only applies to explicit template declarations or definitions}}
+
+template < typename T > struct foo5 { };
+extern template struct  __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5; // expected-error{{must be specified on all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
+template < typename T > struct foo {
+T x;
+T getX() { return x; }
+struct bar {
+T y;
+bar(T y) : y(y) {}
+};
+};
+template < typename T > T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template < typename T > T bar() {
+return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+auto var = foo{5};
+auto var2 = foo::bar{5};
+auto x = bar();
+return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7330,20 +7330,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7397,14 +7399,7 @@
   }
 }
 
-// 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 {
-
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
   }
 
   return Specialization;
@@ -7855,11 +7850,34 @@
   return (Decl*) nullptr;
   }
 
+  bool HadDeclaration = Specialization->getTemplateSpecializationKind() ==
+TSK_ExplicitInstantiationDeclaration;
+  bool HadUniqueInstantiation = Specialization->hasAttr();
+  SourceLocation OldUniqueInstantiationLoc;
+  if (HadUniqueInstantiation) {
+OldUniqueInstantiationLoc =
+  Specializ

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro removed rL LLVM as the repository for this revision.
loladiro updated this revision to Diff 36459.
loladiro added a comment.

Rebased, added support for unique_instantiation on explicit function templates 
and fix the case where one record is embedded in another and the outer is 
explicitly instantiated. Add testcases for all of these.


http://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp

Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template < typename T > struct foo1 { };
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template < typename T > struct foo2 { };
+extern template struct foo2; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct foo3 { };
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct __attribute__((unique_instantiation)) foo4 { }; // expected-error{{only applies to explicit template declarations or definitions}}
+
+template < typename T > struct foo5 { };
+extern template struct  __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5; // expected-error{{must be specified on all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
+template < typename T > struct foo {
+T x;
+T getX() { return x; }
+struct bar {
+T y;
+bar(T y) : y(y) {}
+};
+};
+template < typename T > T bar();
+
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK: define void @_ZN3fooIiE3barC2Ei
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr void @_ZN3fooIiE3barC2Ei
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+extern template __attribute__((unique_instantiation)) int bar();
+
+template < typename T > T bar() {
+return (T)0;
+}
+
+// CHECK: define i32 @_Z3barIiET_v()
+// CHECK-NOT: define weak_odr i32 @_Z3barIiET_v()
+template __attribute__((unique_instantiation)) int bar();
+
+int footest() {
+auto var = foo{5};
+auto var2 = foo::bar{5};
+auto x = bar();
+return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7330,20 +7330,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7397,14 +7399,7 @@
   }
 }
 
-// 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 {
-
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
   }
 
   return Specialization;
@@ -7855,11 +7850,34 @@
   return (Decl*) nullptr;
   }
 
+  bool HadDeclaration = Specialization->getTemplateSpecializationKind() ==
+TSK_ExplicitInstantiationDeclaration;
+  bool HadUniqueInstantiation = Specialization->

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

Thoughts on allowing this attribute to be specified on the templated class 
itself, with the intention of never allowing any implicit instantiation? As an 
example, consider SymbolTableListTraits in LLVM. It can only ever be used as an 
explicit instantiation (because the full implementation is not available).


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

On Fri, Oct 2, 2015 at 1:26 AM, Keno Fischer
 wrote:
> loladiro added inline comments.
>
> 
> Comment at: include/clang/Basic/Attr.td:1462
> @@ +1461,3 @@
> +def UniqueInstantiation : InheritableAttr {
> +  let Spellings = [GCC<"unique_instantiation">];
> +  let Subjects = SubjectList<[CXXRecord]>;
> 
> loladiro wrote:
>> aaron.ballman wrote:
>> > This is not a GCC attribute, so it should not be spelled as such. Since 
>> > this only applies to C++ code, I would recommend a C++11 spelling (in the 
>> > clang namespace). If you think this is something C++03 and earlier code 
>> > would really benefit from, then you could also add a GNU-style spelling.
>> No, this is C++11 only. Will change the spelling.
> It doesn't appear like the grammar allows attributes to be specified on 
> explicit template definitions. I'll change the spelling to GNU:

Ah shoot, that's right. I hadn't noticed  [dcl.attr.grammar]p4.
Specifically, the strangely easily-understandable sentence: No
attribute-specifier-seq shall appertain to an explicit instantiation.

Sorry about that!

~Aaron

>
> ```
> error:
>   an attribute list cannot appear here
> template struct foo [[clang::unique_instantiation]];
>  ^~~
> [[clang::unique_instantiation]]
> error:
>   an attribute list cannot appear here
> template struct [[clang::unique_instantiation]] foo;
> ^~~
> error:
>   an attribute list cannot appear here
> template [[clang::unique_instantiation]] struct foo;
>  ^~~
> error:
>   an attribute list cannot appear here
> [[clang::unique_instantiation]] template struct foo;
> ^~~
> 1 error generated.
> FileCheck error: '-' is empty.
> ```
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13330
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 36332.
loladiro added a comment.

Address review comments. I had to add a special case to 
checkNewAttributesAfterDef if we want to use attribute merging for explicit 
template instantiations, because the Microsoft ABI allows adding dll attributes 
to the explicit template definition, but not the declaration (which clang 
considers to be the record's definition).


Repository:
  rL LLVM

http://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/AttributeList.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp

Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template < typename T > struct foo1 { };
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template < typename T > struct foo2 { };
+extern template struct foo2; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct foo3 { };
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct __attribute__((unique_instantiation)) foo4 { }; // expected-error{{only applies to explicit template declarations or definitions}}
+
+template < typename T > struct foo5 { };
+extern template struct  __attribute__((unique_instantiation)) foo5; // expected-note{{previous explicit instantiation is here}}
+template struct foo5; // expected-error{{must be specified on all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
+template < typename T > struct foo {
+T x;
+T getX() { return x; }
+};
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+int footest() {
+auto var = foo{5};
+return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7321,20 +7321,22 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl)
+mergeDeclAttributes(Specialization, PrevDecl);
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7388,14 +7390,7 @@
   }
 }
 
-// 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 {
-
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
   }
 
   return Specialization;
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4529,6 +4529,29 @@
 Attr.getAttributeSpellingListIndex()));
 }
 
+
+static void handleUniqueInstantiation(Sema &S, Decl *D,
+  const AttributeList &Attr) {
+  if (auto *CTSD = dyn_cast(D)) {
+// If this is an explicit instantiation definition. Check that it was preceeded
+// by an ExplicitInstantiationDeclaration. Note, this
+// requirement encourages a programming style that uses unique explicit
+// instantiation declarations

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.


Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+  let Spellings = [GCC<"unique_instantiation">];
+  let Subjects = SubjectList<[CXXRecord]>;

loladiro wrote:
> aaron.ballman wrote:
> > This is not a GCC attribute, so it should not be spelled as such. Since 
> > this only applies to C++ code, I would recommend a C++11 spelling (in the 
> > clang namespace). If you think this is something C++03 and earlier code 
> > would really benefit from, then you could also add a GNU-style spelling.
> No, this is C++11 only. Will change the spelling.
It doesn't appear like the grammar allows attributes to be specified on 
explicit template definitions. I'll change the spelling to GNU:

```
error:
  an attribute list cannot appear here
template struct foo [[clang::unique_instantiation]];
 ^~~
[[clang::unique_instantiation]]
error:
  an attribute list cannot appear here
template struct [[clang::unique_instantiation]] foo;
^~~
error:
  an attribute list cannot appear here
template [[clang::unique_instantiation]] struct foo;
 ^~~
error:
  an attribute list cannot appear here
[[clang::unique_instantiation]] template struct foo;
^~~
1 error generated.
FileCheck error: '-' is empty.
```


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

On Thu, Oct 1, 2015 at 1:09 PM, Keno Fischer
 wrote:
> loladiro added a comment.
>
> Thanks for the quick review.
>
>
> 
> Comment at: include/clang/Basic/Attr.td:1462
> @@ +1461,3 @@
> +def UniqueInstantiation : InheritableAttr {
> +  let Spellings = [GCC<"unique_instantiation">];
> +  let Subjects = SubjectList<[CXXRecord]>;
> 
> aaron.ballman wrote:
>> This is not a GCC attribute, so it should not be spelled as such. Since this 
>> only applies to C++ code, I would recommend a C++11 spelling (in the clang 
>> namespace). If you think this is something C++03 and earlier code would 
>> really benefit from, then you could also add a GNU-style spelling.
> No, this is C++11 only. Will change the spelling.
>
> 
> Comment at: include/clang/Basic/Attr.td:1464
> @@ +1463,3 @@
> +  let Subjects = SubjectList<[CXXRecord]>;
> +  let Documentation = [Undocumented];
> +}
> 
> aaron.ballman wrote:
>> Please, no undocumented attributes.
> Will document.
>
> 
> Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
> @@ -2442,1 +2442,3 @@
>":must be between 1 and %2}2">;
> +def err_unique_instantiation_wrong_decl : Error<
> +  "unique_instantiation attribute on something that is not a explicit 
> template declaration or instantiation.">;
> 
> aaron.ballman wrote:
>> Would this make more sense as an option in warn_attribute_wrong_decl_type 
>> instead? (there's an err_ version as well if you wish to keep this an error).
>>
>> Also, please ensure that the attribute is quoted in the diagnostic -- it 
>> makes things less confusing for the user.
> Ok, so should I add an "explicit template instantiations" option to that err?

"explicit instantiation definition" seems reasonable if this is
limited to only applying to definitions and not declarations. You
could also automated the checking for this by updating
ClangAttrEmitter.cpp, but I don't think that's a requirement for this
patch.

> 
> Comment at: lib/AST/ASTContext.cpp:8244
> @@ -8242,2 +8243,3 @@
>case TSK_ExplicitInstantiationDefinition:
> -return GVA_StrongODR;
> +CTSD = dyn_cast(FD->getDeclContext());
> +if (!CTSD || !CTSD->hasAttr())
> 
> aaron.ballman wrote:
>> I think this would be easier to read (and not have to hoist a declaration 
>> out of the switch) as:
>>
>> ```
>> if (const auto *CTSD = dyn_cast<>()) {
>>   if (CTSD->hasAttr<>())
>> return GVA_StrongExternal;
>> }
>> return GVA_StrongODR;
>> ```
>>
> Ok.
>
> 
> Comment at: lib/Sema/SemaDeclAttr.cpp:4539
> @@ +4538,3 @@
> +// by an ExplicitInstantiationDeclaration.
> +if (CTSD->getSpecializationKind() == 
> TSK_ExplicitInstantiationDefinition) {
> +  if (!CTSD->getPreviousDecl())
> 
> aaron.ballman wrote:
>> Why is this required as part of the feature design? It seems restrictive.
> This was part of Doug's original Spec, so I implemented it:
>
>
>
>> A unique explicit instantiation definition shall follow an explicit
>> instantiation declaration of the same entity. [//Note//: this
>> requirement encourages a programming style that uses unique explicit
>> instantiation declarations (typically in a header) to suppress
>> implicit instantiations of a template or its members, so that the
>> unique explicit instantiation definition of that template or its members
>> is unique. //- end note//]
>
> I think that makes a decent amount of sense, since you really want to avoid 
> the case where some translation units don't see the extern template 
> declaration.

Okay, that makes sense to me as well. Thank you for pointing that out.
You may want to include this (at least in part) in the comments.

> 
> Comment at: lib/Sema/SemaDeclAttr.cpp:4546
> @@ +4545,3 @@
> +// have performed the same check on the previous declaration here.
> +CXXRecordDecl *Previous = CTSD->getPreviousDecl();
> +if (Previous) {
> 
> aaron.ballman wrote:
>> Is this something that can be handled by mergeDeclAttribute()? I'm not 
>> certain how that interplays with templates specifically, but usually we do 
>> this sort of logic within a Sema::mergeFooAttr() function.
> Hmm, I'm not sure, the goal of this is to ensure that all declarations and 
> definitions of this extern template have the attribute set. It's not really 
> `merging` per se. Though I suppose it could be made to fit in that framework.

Merging is usually the place where we handle "what do other
declarations and definitions of this thing do", which is why it came
to mind. If it turns out that it's an inappropriate place for it, it
would be good to know why.

~Aaron

>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13330
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added a comment.

Thanks for the quick review.



Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+  let Spellings = [GCC<"unique_instantiation">];
+  let Subjects = SubjectList<[CXXRecord]>;

aaron.ballman wrote:
> This is not a GCC attribute, so it should not be spelled as such. Since this 
> only applies to C++ code, I would recommend a C++11 spelling (in the clang 
> namespace). If you think this is something C++03 and earlier code would 
> really benefit from, then you could also add a GNU-style spelling.
No, this is C++11 only. Will change the spelling.


Comment at: include/clang/Basic/Attr.td:1464
@@ +1463,3 @@
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Documentation = [Undocumented];
+}

aaron.ballman wrote:
> Please, no undocumented attributes.
Will document.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
@@ -2442,1 +2442,3 @@
   ":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+  "unique_instantiation attribute on something that is not a explicit template 
declaration or instantiation.">;

aaron.ballman wrote:
> Would this make more sense as an option in warn_attribute_wrong_decl_type 
> instead? (there's an err_ version as well if you wish to keep this an error).
> 
> Also, please ensure that the attribute is quoted in the diagnostic -- it 
> makes things less confusing for the user.
Ok, so should I add an "explicit template instantiations" option to that err?


Comment at: lib/AST/ASTContext.cpp:8244
@@ -8242,2 +8243,3 @@
   case TSK_ExplicitInstantiationDefinition:
-return GVA_StrongODR;
+CTSD = dyn_cast(FD->getDeclContext());
+if (!CTSD || !CTSD->hasAttr())

aaron.ballman wrote:
> I think this would be easier to read (and not have to hoist a declaration out 
> of the switch) as:
> 
> ```
> if (const auto *CTSD = dyn_cast<>()) {
>   if (CTSD->hasAttr<>())
> return GVA_StrongExternal;
> }
> return GVA_StrongODR;
> ```
> 
Ok.


Comment at: lib/Sema/SemaDeclAttr.cpp:4539
@@ +4538,3 @@
+// by an ExplicitInstantiationDeclaration.
+if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
+  if (!CTSD->getPreviousDecl())

aaron.ballman wrote:
> Why is this required as part of the feature design? It seems restrictive.
This was part of Doug's original Spec, so I implemented it:



> A unique explicit instantiation definition shall follow an explicit
> instantiation declaration of the same entity. [//Note//: this
> requirement encourages a programming style that uses unique explicit
> instantiation declarations (typically in a header) to suppress
> implicit instantiations of a template or its members, so that the
> unique explicit instantiation definition of that template or its members
> is unique. //- end note//]

I think that makes a decent amount of sense, since you really want to avoid the 
case where some translation units don't see the extern template declaration.



Comment at: lib/Sema/SemaDeclAttr.cpp:4546
@@ +4545,3 @@
+// have performed the same check on the previous declaration here.
+CXXRecordDecl *Previous = CTSD->getPreviousDecl();
+if (Previous) {

aaron.ballman wrote:
> Is this something that can be handled by mergeDeclAttribute()? I'm not 
> certain how that interplays with templates specifically, but usually we do 
> this sort of logic within a Sema::mergeFooAttr() function.
Hmm, I'm not sure, the goal of this is to ensure that all declarations and 
definitions of this extern template have the attribute set. It's not really 
`merging` per se. Though I suppose it could be made to fit in that framework.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

Re: [PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added reviewers: rsmith, aaron.ballman.


Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+  let Spellings = [GCC<"unique_instantiation">];
+  let Subjects = SubjectList<[CXXRecord]>;

This is not a GCC attribute, so it should not be spelled as such. Since this 
only applies to C++ code, I would recommend a C++11 spelling (in the clang 
namespace). If you think this is something C++03 and earlier code would really 
benefit from, then you could also add a GNU-style spelling.


Comment at: include/clang/Basic/Attr.td:1464
@@ +1463,3 @@
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Documentation = [Undocumented];
+}

Please, no undocumented attributes.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
@@ -2442,1 +2442,3 @@
   ":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+  "unique_instantiation attribute on something that is not a explicit template 
declaration or instantiation.">;

Would this make more sense as an option in warn_attribute_wrong_decl_type 
instead? (there's an err_ version as well if you wish to keep this an error).

Also, please ensure that the attribute is quoted in the diagnostic -- it makes 
things less confusing for the user.


Comment at: lib/AST/ASTContext.cpp:8244
@@ -8242,2 +8243,3 @@
   case TSK_ExplicitInstantiationDefinition:
-return GVA_StrongODR;
+CTSD = dyn_cast(FD->getDeclContext());
+if (!CTSD || !CTSD->hasAttr())

I think this would be easier to read (and not have to hoist a declaration out 
of the switch) as:

```
if (const auto *CTSD = dyn_cast<>()) {
  if (CTSD->hasAttr<>())
return GVA_StrongExternal;
}
return GVA_StrongODR;
```



Comment at: lib/AST/ASTContext.cpp:8353
@@ -8342,2 +8352,3 @@
   case TSK_ExplicitInstantiationDefinition:
-return GVA_StrongODR;
+CTSD = dyn_cast(VD->getDeclContext());
+if (!CTSD || !CTSD->hasAttr())

Similar suggestion here.


Comment at: lib/Sema/SemaDeclAttr.cpp:4535
@@ +4534,3 @@
+  const AttributeList &Attr) {
+  ClassTemplateSpecializationDecl *CTSD;
+  if ((CTSD = dyn_cast(D))) {

The declaration does not need to be hoisted here.


Comment at: lib/Sema/SemaDeclAttr.cpp:4539
@@ +4538,3 @@
+// by an ExplicitInstantiationDeclaration.
+if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
+  if (!CTSD->getPreviousDecl())

Why is this required as part of the feature design? It seems restrictive.


Comment at: lib/Sema/SemaDeclAttr.cpp:4546
@@ +4545,3 @@
+// have performed the same check on the previous declaration here.
+CXXRecordDecl *Previous = CTSD->getPreviousDecl();
+if (Previous) {

Is this something that can be handled by mergeDeclAttribute()? I'm not certain 
how that interplays with templates specifically, but usually we do this sort of 
logic within a Sema::mergeFooAttr() function.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330



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

[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro created this revision.
loladiro added a reviewer: doug.gregor.
loladiro added a subscriber: cfe-commits.
loladiro set the repository for this revision to rL LLVM.

This implements a proposal by Doug Gregor on cfe-dev a couple of years ago, to 
allow the compiler to emit strong symbols for explicit template instantiations. 
Doug's spec, which I have tried to follow can be found here: 
http://lists.llvm.org/pipermail/cfe-dev/attachments/20111203/5e1c6c35/attachment.html.
 I usually work over in LLVM-land and have only contributed the occasional bug 
fix to clang, so please let me know how this patch can be improved. I'm also 
not fully done testing it yet (only did so on the toy example I included as a 
test case), but I wanted to get this out there to get feedback. 




Repository:
  rL LLVM

http://reviews.llvm.org/D13330

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/unique-instantiation.cpp
  test/SemaCXX/unique-instantiations.cpp

Index: test/SemaCXX/unique-instantiations.cpp
===
--- /dev/null
+++ test/SemaCXX/unique-instantiations.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang -cc1 -std=c++11 -fsyntax-only -verify %s
+
+template < typename T > struct foo1 { };
+template struct __attribute__((unique_instantiation)) foo1; // expected-error{{requires a previous declaration}}
+
+template < typename T > struct foo2 { };
+extern template struct foo2; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo2; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct foo3 { };
+extern template struct __attribute__((unique_instantiation)) foo3; // expected-note{{previous explicit instantiation is here}}
+extern template struct foo3; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct __attribute__((unique_instantiation)) foo4 { }; // expected-error{{not a explicit template declaration}}
+
+template < typename T > struct foo5 { };
+extern template struct foo5; // expected-note{{previous explicit instantiation is here}}
+template struct __attribute__((unique_instantiation)) foo5; // expected-error{{must be specified on all declarations}}
+
+template < typename T > struct foo6 { };
+extern template struct  __attribute__((unique_instantiation)) foo6; // expected-note{{previous explicit instantiation is here}}
+template struct foo6; // expected-error{{must be specified on all declarations}}
Index: test/CodeGenCXX/unique-instantiation.cpp
===
--- /dev/null
+++ test/CodeGenCXX/unique-instantiation.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
+template < typename T > struct foo {
+T x;
+T getX() { return x; }
+};
+// CHECK: define i32 @_ZN3fooIiE4getXEv
+// CHECK-NOT: define weak_odr i32 @_ZN3fooIiE4getXEv
+extern template struct __attribute__((unique_instantiation)) foo;
+template struct __attribute__((unique_instantiation)) foo;
+
+int footest() {
+auto var = foo{5};
+return var.getX();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -7321,20 +7321,25 @@
   Specialization->setExternLoc(ExternLoc);
   Specialization->setTemplateKeywordLoc(TemplateLoc);
   Specialization->setRBraceLoc(SourceLocation());
+  Specialization->setTemplateSpecializationKind(TSK);
 
   if (Attr)
 ProcessDeclAttributeList(S, Specialization, Attr);
 
+  if (PrevDecl && PrevDecl->hasAttr() &&
+  !Specialization->hasAttr()) {
+Diag(Specialization->getLocStart(),diag::err_unique_instantiation_not_previous);
+Diag(PrevDecl->getLocStart(),diag::note_previous_explicit_instantiation);
+  }
+
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
   // just put it into the declaration context directly.
   Specialization->setLexicalDeclContext(CurContext);
   CurContext->addDecl(Specialization);
 
   // Syntax is now OK, so return if it has no other effect on semantics.
   if (HasNoEffect) {
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
 return Specialization;
   }
 
@@ -7388,14 +7393,7 @@
   }
 }
 
-// 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 {
-
-// Set the template specialization kind.
-Specialization->setTemplateSpecializationKind(TSK);
   }
 
   return