[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<int>;
> ```
> ```
> // in source file
> template struct X<int>;
> ```
> 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<ClassTemplateSpecializationDecl>(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<UniqueInstantiationAttr>())
+    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 members that are defined at the point where the explicit instantiation appears. It seems like the most consistent behavior is probably to add the attributes to the defined class members as part of the process of explicitly instantiating them, in `InstantiateClassMembers`.
Ok, I'll try that.


================
Comment at: lib/Sema/SemaTemplate.cpp:8063-8066
+      DiagnoseUniqueInstantiation(*this, D, TSK, HadDeclaration,
+                                  Prev->getAttr<UniqueInstantiationAttr>(),
+                                  OldUniqueInstantiation,
+                                  OldUniqueInstantiationLoc);
----------------
rsmith wrote:
> Do we need this to be here as a special case, duplicated for each kind of decl that can have these attributes, rather than handling it using the normal attribute-handling mechanism in SemaDeclAttr? If so, why?
IIRC, the problem was that the new attributes get added to the same node, so we lose the information of whether the attribute was present on the previous declaration.


Repository:
  rL LLVM

https://reviews.llvm.org/D13330