[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1268
+  // Instantiate the attributes on both the template declaration and the associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs, StartingScope);
----------------
ldionne wrote:
> erik.pilkington wrote:
> > Why are you instantiating the attributes onto the ClassTemplateDecl? At the very least it seems wrong to instantiate attributes from the pattern to the ClassTemplateDecl.
> My understanding was that
> - `Inst` represented `B<int>::X<T>` (the template)
> - `RecordInst` represented `B<int>::X<int>` (the template specialization, which is a "real struct")
> - `Pattern`  represented `B<T>::X<T>`
> 
> If that is right, then it seemed to make sense that both `B<int>::X<T>` and `B<int>::X<int>` should have the attribute, since `B<T>::X<T>` has it. I might be misunderstanding something though.
That's almost right.

  * `Inst` represents `B<int>::X` (the template declaration in the instantiation of `B<int>`).
  * `RecordInst` represents `B<int>::X<U>` (the class declaration within the template declaration in the instantiation of `B<int>`).
  * `Pattern` represents `B<T>::X<U>` (the class declaration within the template declaration in the definition of `B<int>`).

And we don't yet have a value for `U`, so there is no `B<int>::X<int>` here.

That is:

```
template<typename T>
  struct B {
    template<typename U> // #1
      struct X; // Pattern
  };
// implicit instantiation of B<int>:
template<> struct B<int> {
  template<typename U> // Inst
    struct X; // RecordInst
};
```

A template-declaration can never have attributes on it (there is no syntactic location where they can be written), so we should never be instantiating attributes onto it. If it could, we should instantiate those attributes from `#1`, not from `Pattern`.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3247
   InstPartialSpec->setTypeAsWritten(WrittenTy);
 
   // Check the completed partial specialization.
----------------
erik.pilkington wrote:
> ldionne wrote:
> > I tried adding
> > 
> > ```
> > SemaRef.InstantiateAttrsForDecl(TemplateArgs, ClassTemplate->getTemplatedDecl(), InstPartialSpec, LateAttrs, StartingScope);
> > ```
> > 
> > here, but just like for explicit specializations, that doesn't instantiate the attributes on the `CXXRecordDecl` used to determine mangling.
> > 
> You mean `SemaRef.InstantiateAttrsForDecl(TemplateArgs, PartialSpec, InstPartialSpec, LateAttrs, StartingScope);`?
erik's suggestion would be the right thing to do. This case is for instantiating a class-scope partial specialization inside a template to form another class-scope partial specialization:

```
template<typename T> struct A {
  template<typename U> struct B;
  template<typename U> struct ATTRS B<U*>; // PartialSpec
};
// implicit instantiation of A<int> looks like:
template<> struct A<int> {
  template<typename U> struct B;
  template<typename U> struct ATTRS B<U*>; // InstPartialSpec
};
```

... where we want to instantiate attributes from `PartialSpec` to `InstPartialSpec`. Unlike in the class template case, we don't have separate AST nodes for the template and the class within it for partial specializations (I think our general consensus is that's a design mistake, but addressing it would be a substantial refactoring).


================
Comment at: clang/test/SemaCXX/PR38913.cpp:19
+};
+C<int>::X<int, long>* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+
----------------
erik.pilkington wrote:
> ldionne wrote:
> > This test is failing in the current iteration of the patch.
> I think the problem here is that we don't instantiate X<int, long> because it's behind the pointer, so we're just considering the primary template. Looks like we do add the attribute (with the fix above) here:
> ```
> template<class T> struct C {
>   template<class U, class V> struct X { };
>   template<class V> struct __attribute__((abi_tag("CTAG"))) X<int, V> { };
> };
> C<int>::X<int, long> c() { return 0; }
> ```
> But we don't get the right mangling, for some reason. Note that this problem is present even in the non-member case. 
> 
> I don't think attributes on specializations have particularly good support anyways, so I wouldn't really lose any sleep if we "left this to a follow-up". Maybe @rsmith has some thoughts here.
This should call `TemplateDeclInstantiator::VisitCXXRecordDecl` to create the member class declaration when instantiating the outer class template specialization `C<int>`. I'm not sure why that isn't instantiating the attributes from the declaration.


Repository:
  rC Clang

https://reviews.llvm.org/D51997