[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann marked 2 inline comments as done.
tahonermann added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10110-10111
+    // propagated to the new node prior to instantiation.
+    if (PrevDecl && PrevDecl->hasAttr<MSInheritanceAttr>()) {
+      Specialization->addAttr(PrevDecl->getAttr<MSInheritanceAttr>());
+      Consumer.AssignInheritanceModel(Specialization);
----------------
rnk wrote:
> `hasAttr()` is O(n) in the attribute list size, so I would recommend this pattern:
> ```
>   if (PrevDecl) {
>     if (const auto *A = PrevDecl->getAttr<MSInheritanceAttr>()) {
>       Specialization->addAttr(A);
>       ...
> ```
Sounds good, I'll make that change.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+      Specialization->addAttr(PrevDecl->getAttr<MSInheritanceAttr>());
+      Consumer.AssignInheritanceModel(Specialization);
+    }
----------------
rnk wrote:
> tahonermann wrote:
> > erichkeane wrote:
> > > tahonermann wrote:
> > > > I am concerned that moving the call to `Consumer.AssignInheritanceModel()` to immediately after the creation of the node, but before it is populated might be problematic. Previously, this call was still made before the node was completely constructed (e.g., before `setTemplateSpecializationKind()` is called for it). It is difficult to tell if any consumers might be dependent on more of the definition being present. 
> > > SO I think we still need to do this off of the definition, right?  Else if `PrevDecl` ends up being a declaration (followed later by a definition that has the attribute), we'll end up missing it?  Typically attributes are 'added' by later decls, so only the 'latest one' (though attributes can't be added after definition IIRC) has 'them all'.
> > This handles the situation where a new node is created for either an explicit template instantiation declaration or definition; that matches prior behavior. The goal is to ensure each node, regardless of the reason for its creation, inherits the attribute from the previous declaration; that ensures that any explicitly declared attributes are checked for consistency (see `Sema::mergeMSInheritanceAttr()`).
> > 
> > Note that an explicit class template specialization is not allowed to follow an explicit template instantiation declaration or definition. https://godbolt.org/z/cbvaac717.
> I think it is safe to call `AssignInheritanceModel` earlier. We mostly just use it to invalidate some clang AST -> LLVM IR type translation cache.
Great, thank you for confirming that.


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp:958
+extern template class a<int>;
+template class a<int>;
+}
----------------
rnk wrote:
> My expectation here is that we assign the unknown inheritance model to `a<int>`, is that right? Can you add a static_assert to that effect, or add some CHECK lines for the structure,  maybe make an alloca of type `void (a<int>::*var)()`  and check for the allocated type (it should be a struct with a pointer with lots of i32s)?
Yes, when a pointer to member is formed for an incomplete class, in the absence of a `#pragma pointers_to_members` directive, use of inheritance keywords like `__single_inheritance`, or use of the `/vmb` or `/vmg` options (or equivalents), the "full_generality" / "virtual_inheritance" model is selected. I did verify that manually.

I can add a `static_assert` so long as it follows the explicit template instantiation definition. If it is placed earlier, then code related to obtaining a complete type is run and that ends up avoiding the assertion failure. See https://godbolt.org/z/qzTTfdfY1. This might indicate there is a bug elsewhere; I find it surprising that the early `static_assert` has that effect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158869/new/

https://reviews.llvm.org/D158869