[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 6 inline comments as done.
mgehre added inline comments.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:102
+          dyn_cast_or_null<ClassTemplateDecl>(Record->getDescribedTemplate())) {
+    if (auto *Def = Record->getDefinition())
+      addGslOwnerPointerAttributeIfNotExisting<Attribute>(Context, Def);
----------------
gribozavr wrote:
> I wonder why this is necessary. Sema should call inference on every CXXRecordDecl. Is it because of incorrect short-circuiting in `inferGslPointerAttribute` that I'm pointing out below?
The contract is: A CXXRecordDecl is a Pointer/Owner if and only if its canonical declaration has the Pointer/Owner attribute.

The analysis only checks this on concrete classes (excuse me if I'm using the wrong term here), i.e. non-template classes or instantiation of template classes.

During the inference, we might also put the attributes onto the templated declaration of a ClassTemplateDecl. The Pointer/Owner attributes here will not be used by the analysis.
We just put them there so clang will copy them to each instantiation, where they will be used by the analysis.

>From what I understand, clang copies the attributes of the definition of the templated declaration to the instantiation.
In addition, when clang sees a redeclaration (e.g. of a template), it will also copy the attributes from the previous/canonical (?) declaration.

In the problematic case
```
// The canonical declaration of the iterator template is not its definition.
template <typename T>
class __unordered_multiset_iterator;

template <typename T>
class __unordered_multiset_iterator {
};

template <typename T>
class unordered_multiset {
public:
  using iterator = __unordered_multiset_iterator<T>;
};

static_assert(sizeof(unordered_multiset<int>::iterator), ""); // Force instantiation.
```
clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is the canonical declaration, but not a definition)
2. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is not the canonical declaration, but its definition)
3. While processing the `using iterator` line, add a PointerAttr to the canonical declaration (from point 1)
4. While instantiating `__unordered_multiset_iterator<int>`, copy attributes from the template definition (from point 2), which are none
Result: The `ClassTemplateSpecializationDecl` for `__unordered_multiset_iterator<int>` would not have the `PointerAttr`.

On the other hand, a swapped example
```
template <typename T>
class __unordered_multiset_iterator;

template <typename T>
class unordered_multiset {
public:
  using iterator = __unordered_multiset_iterator<T>;
};

template <typename T>
class __unordered_multiset_iterator {
};

static_assert(sizeof(unordered_multiset<int>::iterator), ""); // Force instantiation.
```
would work because clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is the canonical declaration, but not a definition)
2. While processing the `using iterator` line, add a PointerAttr to the canonical declaration (from point 1)
3. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is not the canonical declaration, but its definition); copy all attributes from a previous declaration, i.e. the PointerAttr from point 1
4. While instantiating `__unordered_multiset_iterator<int>`, copy PointerAttr from the template definition (from point 3)

So we cannot just put the PointerAttr on the definition of the class template because there might be none yet (like in the second example). We need to put it on the definition in case
it was already parsed.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:200
     CXXRecordDecl *Canonical = Record->getCanonicalDecl();
     if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
       return;
----------------
gribozavr wrote:
> Should this code not do this short-circuit check? It is prone to going out of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, like it did just now.
> 
> If you want to check for attribute before doing the string search, you could pass the string set into `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it should infer the attribute or not.
Yes, the `hasAttr` check here is an optimization to avoid the string search. I don't think I can move the string search into `addGslOwnerPointerAttributeIfNotExisting`, because that function is called 
from other call sites that don't care about a set of names.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
 
+// The iterator typedef is a DependentNameType.
+template <typename T>
----------------
gribozavr wrote:
> This test file is getting pretty long and subtle, with lots of things are being mixed into one file without isolation.
> 
> WDYT about refactoring it into a unit test that uses AST matchers to verify attribute presence?
> 
> See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples.
> 
> Each test case would look approximately like this:
> 
> ```
> EXPECT_TRUE(matches(
>   "template class ...",
>   classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer)));
> ```
> 
> Each test case would be isolated from other tests, each test would have a name (and optionally a comment) that will make it obvious what exactly is being tested.
> 
> It would be also possible to verify things like "The iterator typedef is a DependentNameType" to ensure that we're testing exactly what we want to test.
I like the AST matcher approach! This test file is really hard to debug - I usually copy a test to its own file for debugging. 
Would you be okay with deferring the testing change to the next PR?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179