gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[mgehre]

Summary

Make the DerefType, i.e. the argument of gsl::Owner/gsl::Pointer optional for now.

The DerefType is used when infering lifetime annotations of functions, e.g.
how a return value can relate to the arguments. Our initial checks should
work without that kind of inference and instead use explicit annotations,
so we don't need the DerefType right now.
And having it optional makes the implicit/default annotations for std:: types
easier to implement. I think the DerefType is also optional in the current MSVC
implementation.

This seems to be the first attribute that has an optional Type argument,
so I needed to fix two things in tablegen:

• Don't crash AST dump when Owner/Pointer has no Type argument
• Don't crash when pretty-printing an Attribute with optional type argument

Add is_gsl_owner/is_gsl_pointer builtins. They are very useful to test
if the annotations have been attached correctly.

Hard code gsl::Owner/gsl::Pointer for std types. The paper mentions
some types explicitly. Generally, all containers and their iterators are
covered. For iterators, we cover both the case that they are defined
as an nested class or as an typedef/using. I have started to test this
implementation against some real standard library implementations, namely
libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

The tests are currently here

https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.sh
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp

I think due to their dependency on a standard library, they are not a good fit
for clang/test/. Where else could I put them?

[gribozavr]

The tests are currently here
I think due to their dependency on a standard library, they are not a good fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and check them into clang/test.

[xazax.hun]

In D64448#1577109, @gribozavr wrote:

The tests are currently here
I think due to their dependency on a standard library, they are not a good fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and check them into clang/test.

Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp. I was thinking about adding the corresponding tests to libc++ as well so we have better chances to get some notifications if a new language feature/implementation trick is introduced.

[gribozavr]

Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp.

I see. Sorry, but that seems insufficient to me -- different libraries use different patterns. For example, libc++ wraps everything in std in an inline namespace. I don't know how various versions of libstdc++ differ.

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I'd suggest to split type trait implementations into a separate patch.

Are these type traits being exposed only for testing? I'm not sure it is a good idea to do that -- people will end up using them in production code -- is that a concern? If so, maybe it would be better to test through warnings.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

This test and related code changes could be split off into a separate patch.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

I feel like these tests would be better off in a separate file.

It would be also good to explain which exact library we are trying to imitate in this test. Different libraries use different coding patterns.

103

Is it actually defined like that?

140

Unclear what this test is testing.

If there's something special about thread (e.g., it looks very much like an owner or a pointer, and a buggy implementation can easily declare thread to be an owner or a pointer), please explain that in a comment.

If you're testing that some random name is not being picked up by inference rules, I'd suggest to use an obviously-fictional name ("class type_unknown_to_compiler;")

[JonasToth]

Just as Sidenote in case you were not aware: There is a check in clang-tidy for gsl::owner (https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html)
It will benefit from your annotation and can be simplified.

[mgehre]

In D64448#1577779, @gribozavr wrote:

For example, libc++ wraps everything in std in an inline namespace.

I believed that I had written a test for inline namespaces, but seems that I shouldn't be working in the middle of the night.
I will add one.

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I copied that approach from https://reviews.llvm.org/D50119.
If people do it in production code, then at least the two leading underscores should tell them "I'm not supposed to use this".

I considered a test through warnings, but how would I trigger them? Add -Wlifetime-categories-debug which emits notes whenever a [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

I was thinking whether it made sense to separate

• fixing the AST dump of attributes with optional type parameter when there is not such attribute
• introduce and attribute with optional type parameter while AST dumping it is broken

so I decided that both are closely related. Otherwise the fix and its test are in separate PRs?

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

This is imitating techniques from different libraries - all techniques that this implementation supports.

To check if all techniques that this implementation supports are enough for real standard library implementations,
I use https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp against them. Various versions of libstdc++
and libc++ passed. I will test MSVC standard library next. If they would use a technique that this implementation does not support yet,
I will add support for that and the corresponding test here.
I might fix MSVC support (if needed) only in the following PR:

103

There might be a standard library implementation that does it like this. This tests that we will use the using iterator = set_iterator<T>; in class set { to attach the [[gsl::Pointer]] to the set_iterator.

140

Agreed, I will rename this to an obviously-fictional name.

[gribozavr]

In D64448#1581719, @mgehre wrote:

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

Yes, I saw the testcase -- but how do different libstdc++ versions differ?

[gribozavr]

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

If people do it in production code, then at least the two leading underscores should tell them "I'm not supposed to use this".

That's not what two underscores mean. These custom type traits, being language extensions, must have a name that won't collide with any user-defined name, hence two underscores. Two underscores mean nothing about whether the user is allowed to use it or not. Sure the code might become non-portable to other compilers, but that's not what the concern is. My concern is that code that people write might become unportable to future versions of Clang, where we would have to change behavior of these type traits (or it would just subtly change in some corner case and we won't notice since we don't consider it a public API).

I considered a test through warnings, but how would I trigger them?

I meant regular warnings, that are the objective of this analysis -- warnings about dereferencing dangling pointers. If we get those warnings for the types-under-test, then obviously they have the correct annotations from the compiler's point of view.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

Totally makes sense to have the fix and the test is the same PR, but they seem to be separable from attribute inference for std types, right?

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

This is imitating techniques from different libraries - all techniques that this implementation supports.

Okay -- please add comments about each technique then (and ideally, which libraries use them). Right now (for me, who didn't write the patch), the test looks like it is testing inference for a bunch of types, not for a bunch of techniques -- the differences are subtle and non-obvious.

103

Then std::set_iterator must have an implementation-reserved name, like std::__set_iterator.

[mgehre]

• Implement comments

[mgehre]

In D64448#1581771, @gribozavr wrote:

In D64448#1581719, @mgehre wrote:

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

Yes, I saw the testcase -- but how do different libstdc++ versions differ?

I didn't know whether they would differ, but the test tells me that they don't differ significantly (i.e. in introducing new techniques).
Would you like me to test with other standard libraries (besides MSVC, which I already planned)?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I spent a lot of time debugging on the full lifetime analysis, and knowing exactly which type has which Attribute (especially if inference is involved) was quite important.
I would say that adding additional code to trigger a real lifetime warning

• requires that we first add some lifetime warnings to clang (currently in a different PR)
• obscures the purpose of the check, i.e. checking the attributes and not the warnings themselves
• and makes debugging hard when the test fails (warnings involve at least one Owner and one Pointer, so either of those could have made the test fail)

I'd prefer if we can test the attributes directly.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

Yes, you are right. And Aaron would like to have the type parameter already optional in D63954, so I will move this part over there.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

Sure, I will add comments.

103

True, I can call it like that to make the test more realistic.

[gribozavr]

In D64448#1581866, @mgehre wrote:

I didn't know whether they would differ, but the test tells me that they don't differ significantly (i.e. in introducing new techniques).

Okay -- I would prefer if you intentionally looked at the code before declaring a library as supported, or asked someone who knows about the differences, but it is your call.

Would you like me to test with other standard libraries (besides MSVC, which I already planned)?

It is your call which libraries you would like to support.

Another question -- in the latest iteration of the patch the inference code disappeared -- was it intentional?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I agree that being able to test these properties definitely simplifies testing. I am worried about adding language extension though, and would like someone like Richard Smith to LGTM this approach.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

s/builtin attributes/Test attribute inference for types in the standard library./

88

s/added to/inferred for/

93

Sorry, but what do you mean by a "concrete template"?

"Attributes are inferred on class template instantiations."

102

lowercase "attributes"

114

lowercase "attributes"

115

Could you separate the inline namespace test from the iterator part into another test, say, for unordered_map?

[rsmith]

clang/include/clang/Basic/Attr.td

2773 ↗

(On Diff #208813)

On the surface this doesn't appear to make sense, but I think that's a pre-existing bug.

@aaron.ballman Is this field name reversed from the intent? I think what it means is "meaningful on (non-defining) class declaration", which is... the opposite of what its name suggests. Looking at the tablegen implementation, it appears that setting this to 1 causes the attribute to be instantiated when instantiating a class declaration, not only when instantiating a class definition.

clang/include/clang/Basic/AttrDocs.td

4167–4169

... and what does the attribute mean when the argument is omitted?

4180

Similarly, what happens in that case?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

If you just want this for testing, could you instead use an -ast-dump test and see if the attributes were added? Alternatively I'd be OK having these as just-for-testing traits if you give them names that makes their status as being for-testing, unsupported, may-be-removed-or-changed-at-any-time clear. (Eg, __clang_debug_is_gsl_owner.)

Generally we would like the __is_* type traits to exactly match the semantics of the corresponding standard C++ std::is_* type traits (except where we're forced to do something else for compatibility).

[mgehre]

• Merge branch 'lifetime-categories' into hardcode-lifetime-categories; Split test; Move test to AST dump; fix for comments;

[mgehre]

This should fix all open comments.

clang/include/clang/Basic/AttrDocs.td

4167–4169

I added text that it is currently ignored. We will use the argument in future analysis, but already add parsing of it for forward compatibility.
Per Aaron's request, this part moved to the parent PR, D63954.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86

I moved the std tests to a different file.

93

It says "complete", and I mean "not forward declared".

[lebedev.ri]

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

[mgehre]

• Remove type traits

[mgehre]

In D64448#1595017, @lebedev.ri wrote:

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

Thanks for the question! The implicitly added attributes have an invalid source location.

[lebedev.ri]

In D64448#1595022, @mgehre wrote:

In D64448#1595017, @lebedev.ri wrote:

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

Thanks for the question! The implicitly added attributes have an invalid source location.

Sounds good, thanks.

[gribozavr]

clang/include/clang/Sema/Sema.h

6097 ↗

(On Diff #211010)

It seems like this function does not add gsl::Owner.

clang/lib/Sema/SemaTemplate.cpp

1689 ↗

(On Diff #211010)

It shouldn't be necessary to perform inference here, instead, the attributes should be instantiated, see instantiateDependentAlignedAttr in SemaTemplateInstantiateDecl.cpp for an example.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

702 ↗

(On Diff #211010)

Ditto, should not be necessary to perform inference here.

[mgehre]

• Fix comments
• Add Pointer via typedef on ClassTemplateDecl

clang/include/clang/Sema/Sema.h

6097 ↗

(On Diff #211010)

Fixed comment

clang/lib/Sema/SemaTemplate.cpp

1689 ↗

(On Diff #211010)

From what I understand, here the attribute is put on the ClassTemplateDecl. Without this, there would be no attribute to instantiate from?
The instantiation of OwnerAttr/PointerAttr is auto-generated into instantiateTemplateAttribute() in build/tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

702 ↗

(On Diff #211010)

I'll move this to already act on the typedef in the ClassTemplateDecl (instead of the instantiation here).

[mgehre]

• Add missing check

[mgehre]

• Fix crash

[gribozavr]

clang/include/clang/Sema/Sema.h

6122 ↗

(On Diff #212446)

s/addDefault/infer/ ? (everywhere in the patch)

clang/include/clang/Basic/AttrDocs.td

 }
 
 def LifetimeOwnerDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
 that returns cv-qualified ``T&`` or ``T*`` is assumed to return a
 pointer/reference to the data owned by ``O``. The owned data is assumed to end
-its lifetime once the owning object's lifetime ends. The argument is ``T`` is
+its lifetime once the owning object's lifetime ends. The argument ``T`` is
 optional.
 
 This attribute may be used by analysis tools but will not have effect on code
 generation.
 }];
 }
 
 def LifetimePointerDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 When annotating a class with ``[[gsl::Pointer(T)]]``, it assumed to be a
 non-owning type whose objects can point to ``T`` type objects or dangle.
-The argument is ``T`` is optional.
+The argument ``T`` is optional.
 
 This attribute may be used by analysis tools but will not have effect on code
 generation.
 
 For example ``std::vector<int>::iterator`` might be annotated
 ``[[gsl::Pointer(int)]]``.
 }];
 }

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

 class [[gsl::Owner()]] [[gsl::Owner(int)]] WithAndWithoutParameter{};
 // expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
 // expected-note@-2 {{conflicting attribute is here}}
 
 static_assert(!__is_gsl_pointer(int), "");
 static_assert(__is_gsl_pointer(int &), "");
 static_assert(__is_gsl_pointer(int *), "");
 
-// Test builtin annotation for std types.
+// Test builtin attributes for std types.
 namespace std {
-// Complete class
+// Attributes are added to a (complete) class.
 class any {
 };
 static_assert(__is_gsl_owner(any), "");
 
-// Complete template
+// Attributes are added to a instantiatons of a complete template.
 template <typename T>
 class vector {
 public:
   class iterator {};
 };
 static_assert(__is_gsl_owner(vector<int>), "");
 static_assert(__is_gsl_pointer(vector<int>::iterator), "");
 
+// If std::container::iterator is a using declaration, Attributes are added to
+// the underlying class
 template <typename T>
-class set_iterator {};
+class __set_iterator {};
 
 template <typename T>
 class set {
 public:
-  using iterator = set_iterator<T>;
+  using iterator = __set_iterator<T>;
 };
 static_assert(__is_gsl_pointer(set<int>::iterator), "");
 
+// If std::container::iterator is a typedef, Attributes are added to the
+// underlying class. Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
 template <typename T>
-class map_iterator {};
+class __map_iterator {};
 
 template <typename T>
 class map {
 public:
-  typedef map_iterator<T> iterator;
+  typedef __map_iterator<T> iterator;
 };
+} // namespace inlinens
 static_assert(__is_gsl_pointer(map<int>::iterator), "");
 
-// list has an implicit gsl::Owner attribute,
+// std::list has an implicit gsl::Owner attribute,
 // but explicit attributes take precedence.
 template <typename T>
 class [[gsl::Pointer]] list{};
 static_assert(!__is_gsl_owner(list<int>), "");
 static_assert(__is_gsl_pointer(list<int>), "");
 
-// Forward declared template
+// Forward declared template (Owner)
 template <
     class CharT,
     class Traits>
 class basic_regex;
 static_assert(__is_gsl_owner(basic_regex<char, void>), "");
 
+// Forward declared template (Pointer)
 template <class T>
 class reference_wrapper;
 static_assert(__is_gsl_pointer(reference_wrapper<char>), "");
 
-class thread;
-static_assert(!__is_gsl_pointer(thread), "");
-static_assert(!__is_gsl_owner(thread), "");
+class some_unknown_type;
+static_assert(!__is_gsl_pointer(some_unknown_type), "");
+static_assert(!__is_gsl_owner(some_unknown_type), "");
 } // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't add implicit attributes.
+class any {
+};
+static_assert(!__is_gsl_owner(any), "");
+} // namespace user