[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > Has Microsoft already added support for this? We had an unfortunate problem with `gsl::suppress` where we introduced the attribute first and MSVC introduced it under a different, incompatible syntax. I would strongly like to avoid repeating that.
> > > > I will double check this. Her Sutter's paper have this spelling and I believe that the Microsoft implementation is following the same spelling. The main difference is that the MSVC version does not have a type argument at this point but they do plan to add it as an optional argument. (They want to infer the pointee type when it is not provided.) The clang community do not want the inference, hence we made the type argument required. Always providing the type argument should be compatible with future versions of the MSVC implementation.
> > > I take this to mean that we're not implementing the same attribute as MSVC is, and that worries me.
> > > 
> > > > Always providing the type argument should be compatible with future versions of the MSVC implementation.
> > > 
> > > The problem is that code written for MSVC (where the type argument is not required) will not compile with Clang (where the type argument is required), if I understand properly.
> > > 
> > > Generally speaking, when we implement an attribute from another vendor namespace, we expect the attribute to have the same semantics as whoever "owns" that vendor namespace. It's a bit trickier here because `gsl` isn't really a vendor so much as a collective, so I don't know who is the authority to turn to.
> > > 
> > > On a completely different note: would this attribute also make sense within C with a C2x spelling?
> > I see, that is a valid concern. A followup patch makes the type argument optional so it will be fully compatible with MSVC.  I don't think it makes sense with C or at least I am not sure what would I annotate using these attributes in C code as the subjects are classes. I know that it is possible to simulate classes in C but I am not sure how well the analysis would work in such cases. 
> > 
> > Are you OK with making the argument optional in a followup patch or do you want us to cherry-pick that modification into this one?
> > I don't think it makes sense with C or at least I am not sure what would I annotate using these attributes in C code as the subjects are classes. I know that it is possible to simulate classes in C but I am not sure how well the analysis would work in such cases.
> 
> I couldn't think of a reason to expose this in C either, so I think the current spelling is good as-is.
> 
> > Are you OK with making the argument optional in a followup patch or do you want us to cherry-pick that modification into this one?
> 
> I'd prefer the argument be optional in this patch.
The argument is now optional.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  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
----------------
gribozavr wrote:
> Slightly better: "In a class annotated with ..., each function that returns ... is assumed to ..."
> 
> However, the "is assumed" part throws me off. There's no assumption here -- this is what this attribute means, intentionally, by design, no guessing involved.
> 
> So how about this?
> 
> The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own an object of type `T`:
> 
> ```
> [[gsl::Owner(int)]] class IntOwner {
> private:
>   int value;
> public:
>   int *getInt() { return &value; }
>   long *getLong();
> };
> ```
> 
> In a class annotated like this each member function that returns `T&` or `T*` returns a pointer/reference to the data owned by the class instance:
> 
> ```
> IntOwner int_owner;
> int *x = int_owner.getInt();
> // `x` points to memory owned by `int_owner`.
> ```
> 
> Therefore, that the lifetime of that data ends when ... For example:
> 
> ```
> int *x = IntOwner().getInt();
> // `x` points to memory owned by the temporary `IntOwner()`, which has been destroyed now.
> 
> long *y = IntOwner().getLong();
> // Can't make any conclusion about validity of `y`.
> ```
> 
What I meant to express by "is assumed to" is that this attribute does not change what member functions do. E.g. if I had `int* f() { static int i; return &i; }`, then `f` would not return data owned by the class instance, even when annotating the class with `[[gsl::Owner(T)]]`. The point is that when member functions with return type `T*`/`T&` don't return data owned by the class instance, one should not add the `[[gsl::Owner(T)]]` attribute - or the analysis will produce wrong results. Here, we would warn if code uses the returned value of `f` after the class instances has been destroyed even though it would be safe.

There is a chicken and egg problem here. With this PR, we don't get any analysis yet, so providing examples of analysis results like "`x` points to memory owned by the temporary `IntOwner()`, which has been destroyed now." can be misleading. On the other hand, if we would just document the current meaning of the attribute, it would be nothing.
The implication "member function that returns `T&` or ` T*` returns a pointer/reference to the data owned by the class instance" is just one part of being an Owner. Herb's paper
uses this in more ways (e.g. output arguments, handling of move semantics, etc.), which we would implement incrementally together with the respective documentation update.

So I propose to keep the documentation basic for this PR and update the documentation with the upcoming PRs that add to the analysis. Would that be acceptable?
I added a note that the attribute is currently experimental and subject to change.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:335-341
+  if (attributeIsTypeArgAttr(*AttrName)) {
+    ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, EndLoc, ScopeName,
+                              ScopeLoc, Syntax);
+    // FIXME: when attributeIsTypeArgAttr() is true, assumes that the attribute
+    // takes a single parameter.
+    return 1;
+  }
----------------
aaron.ballman wrote:
> I don't like how this short-circuits the remainder of the logic in this function. I'd rather see this one-off logic handled in `ParseClangAttributeArgs()` if we have to treat it as a one-off. A better approach would be to implement type arguments within `ParseAttributeArgsCommon()`.
I integrated parsing of type attributes now into the main logic of `ParseAttributeArgsCommon()`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2618-2619
+    } else if (isa<OwnerAttr>(NewAttribute) || isa<PointerAttr>(NewAttribute)) {
+      // gsl::Owner and gsl::Pointer are allowed to be added to a class after it
+      // is defined.
+      ++I;
----------------
aaron.ballman wrote:
> I'd like to see some tests for this.
> 
> I'm a bit worried about whether this will do good things in practice or not. I'm concerned about situations like:
> ```
> struct S;
> 
> void func(S *s) {
>   // Interesting things happen here.
> }
> 
> struct [[gsl::Pointer]] S {};
> ```
> and whether the "interesting things" will be properly [un]diagnosed.
I have removed this part of the PR. Implementing implicit attributes for `std` types went well without requiring to allow "late" additions of attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954