[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

Thu Jul 11 16:10:18 PDT 2019

mgehre added a comment.

In D64448#1581771 <https://reviews.llvm.org/D64448#1581771>, @gribozavr wrote:

> In D64448#1581719 <https://reviews.llvm.org/D64448#1581719>, @mgehre wrote:
>
> > In D64448#1577779 <https://reviews.llvm.org/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)?

================
Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type           , KEYCXX)
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > 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.
> > 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?
> > 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.
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.

================
Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > This test and related code changes could be split off into a separate patch.
> > 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?
> 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?
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.

================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > 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.
> > 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:
> > 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.
Sure, I will add comments.

================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103
+template <typename T>
+class set_iterator {};
+
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > Is it actually defined like that?
> > 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`. 
> Then std::set_iterator must have an implementation-reserved name, like std::__set_iterator.
True, I can call it like that to make the test more realistic.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448