Page MenuHomePhabricator

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

Authored by mgehre on Jul 9 2019, 2:33 PM.

Details

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?

Event Timeline

mgehre created this revision.Jul 9 2019, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 2:33 PM
Herald added a subscriber: rnkovacs. · View Herald Transcript

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 edited the summary of this revision. (Show Details)Jul 9 2019, 3:21 PM

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.

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 ↗(On Diff #208813)

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 ↗(On Diff #208813)

Is it actually defined like that?

140 ↗(On Diff #208813)

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;")

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 marked 5 inline comments as done.Jul 11 2019, 3:24 PM

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.

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 ↗(On Diff #208813)

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 ↗(On Diff #208813)

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 ↗(On Diff #208813)

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

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 added inline comments.Jul 11 2019, 3:45 PM
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 ↗(On Diff #208813)

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 ↗(On Diff #208813)

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

mgehre updated this revision to Diff 209370.Jul 11 2019, 4:09 PM
mgehre marked 9 inline comments as done.
  • Implement comments

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 ↗(On Diff #208813)

Sure, I will add comments.

103 ↗(On Diff #208813)

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

mgehre marked an inline comment as done.Jul 11 2019, 4:12 PM

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 ↗(On Diff #209370)

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

88 ↗(On Diff #209370)

s/added to/inferred for/

93 ↗(On Diff #209370)

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

"Attributes are inferred on class template instantiations."

102 ↗(On Diff #209370)

lowercase "attributes"

114 ↗(On Diff #209370)

lowercase "attributes"

115 ↗(On Diff #209370)

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

rsmith added inline comments.
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–4168 ↗(On Diff #208813)

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

4180 ↗(On Diff #208813)

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 updated this revision to Diff 211009.Jul 21 2019, 2:25 PM
mgehre marked 12 inline comments as done.
  • Merge branch 'lifetime-categories' into hardcode-lifetime-categories; Split test; Move test to AST dump; fix for comments;

This should fix all open comments.

clang/include/clang/Basic/AttrDocs.td
4167–4168 ↗(On Diff #208813)

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 ↗(On Diff #208813)

I moved the std tests to a different file.

93 ↗(On Diff #209370)

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

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 updated this revision to Diff 211010.Jul 21 2019, 2:38 PM
  • Remove type traits

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.

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 added inline comments.Jul 29 2019, 7:37 AM
clang/include/clang/Sema/Sema.h
6097

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

clang/lib/Sema/SemaTemplate.cpp
1689

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

Ditto, should not be necessary to perform inference here.

mgehre updated this revision to Diff 212439.Jul 30 2019, 2:24 PM
mgehre marked 6 inline comments as done.
  • Fix comments
  • Add Pointer via typedef on ClassTemplateDecl
clang/include/clang/Sema/Sema.h
6097

Fixed comment

clang/lib/Sema/SemaTemplate.cpp
1689

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

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

mgehre updated this revision to Diff 212441.Jul 30 2019, 2:26 PM
  • Add missing check
mgehre updated this revision to Diff 212446.Jul 30 2019, 2:55 PM
  • Fix crash
gribozavr accepted this revision.Aug 6 2019, 4:09 AM
gribozavr added inline comments.
clang/include/clang/Sema/Sema.h
6091

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

This revision is now accepted and ready to land.Aug 6 2019, 4:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 3:46 AM