[attributes] Add an attribute "os_consumes_this" with similar semantics to "ns_consumes_self"

[george.karpenkov]

Summary

The attribute specifies that the call of the C++ method consumes reference to "this".

[george.karpenkov]

I know that the error message is not quite right, but this has nothing to do with this patch.
I'll see on which level it could be fixed: currently the error for mismatch for CXXMethodDecl still prints "function".

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

887 ↗

(On Diff #176230)

Similarly -> Similar

clang/test/Sema/attr-osobject.cpp

46 ↗

(On Diff #176230)

Missing some more test coverage:

• Attribute takes no args
• Attribute does not appertain to a static member function

Question: what should happen in this code?

struct S {
  virtual void frobble() [[clang::os_consumes_this]];
};

struct T : S {
  void frobble() override;
};

int main() {
  S *s = new T;
  T *t = static_cast<T *>(s);

  s->frobble();
  t->frobble();
}

Does the attribute get inherited by the overrider?

(This question really applies to the other attributes in this area as well, but it cropped up while I was thinking about this pointers.)

[george.karpenkov]

Does the attribute get inherited by the overrider?

Yes conceptually: https://reviews.llvm.org/D55154, we propagate it in the analyzer.
No in the AST: I don't think it makes sense to propagate it during AST construction

[aaron.ballman]

clang/include/clang/Basic/Attr.td

851 ↗

(On Diff #176909)

Given how trivial the implementation is in SemaDeclAttr.cpp, I think a better approach would be to add a new SubsetSubject similar to NonStaticNonConstCXXMethod and use that as the subject list here. Then you can remove the new custom diagnostic and handler as well. Also, shouldn't this be limited to member functions, not non-member functions?

852 ↗

(On Diff #176909)

What do you think about splitting this out from the retain docs into consume docs (same for the two attributes below)? Or is there enough overlap between the two sections that you'd just wind up duplicating a lot of content?

clang/include/clang/Basic/AttrDocs.td

889 ↗

(On Diff #176909)

I'd drop the second e.g., as it reads strangely. Also, it should be e.g., for the first one.

clang/test/Sema/attr-osobject.cpp

8 ↗

(On Diff #176909)

What this pointer is being consumed here? I would have expected this to have a diagnostic.

51 ↗

(On Diff #176909)

But... this is a function?

Also, spurious semi-colon after the function body.

[george.karpenkov]

clang/include/clang/Basic/Attr.td

851 ↗

(On Diff #176909)

I'm a bit confused: aren't CXXMethods already member functions by definition?

852 ↗

(On Diff #176909)

Yeah I think they should be grouped together.

clang/test/Sema/attr-osobject.cpp

8 ↗

(On Diff #176909)

What do you mean? This function does not have a body, but a valid implementation could be e.g. this->release()

[aaron.ballman]

Looks close, just a few minor testing concerns.

clang/include/clang/Basic/Attr.td

851 ↗

(On Diff #176909)

They are, but they're not necessarily always instance methods. For instance, they can be static member functions without a this pointer. I suppose I used confusing terminology though by talking about member vs nonmember, sorry about that.

852 ↗

(On Diff #176909)

As you've seen, that causes some duplication in the resulting documentation. I suspect we may want to refactor the docs at some point, but that shouldn't hold up this patch.

clang/test/Sema/attr-osobject.cpp

8 ↗

(On Diff #176909)

That's my fault -- my eyes glazed over that this was inside the structure declaration -- I thought the }; was a line earlier for some reason. :-P

Can you add a second test that uses a static member function, which should be diagnosed.

51 ↗

(On Diff #176909)

Still has the spurious semicolon.

[george.karpenkov]

done

[aaron.ballman]

LGTM!

include/clang/Basic/Attr.td

def NonParmVar : SubsetSubject<Var,
                                "variables">;
 def NonLocalVar : SubsetSubject<Var,
                                 [{!S->hasLocalStorage()}],
                                 "variables with non-local storage">;
 def NonBitField : SubsetSubject<Field,
                                 [{!S->isBitField()}],
                                 "non-bit-field non-static data members">;
 
+def NonStaticCXXMethod : SubsetSubject<CXXMethod,
+                                       [{!S->isStatic()}],
+                                       "non-static member functions">;
+
 def NonStaticNonConstCXXMethod
     : SubsetSubject<CXXMethod,
                     [{!S->isStatic() && !S->isConst()}],
                     "non-static non-const member functions">;
 
 def ObjCInstanceMethod : SubsetSubject<ObjCMethod,
                                        [{S->isInstanceMethod()}],
                                        "Objective-C instance methods">;

 }
 
 def OSReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"os_returns_not_retained">];
   let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
+def OSConsumesThis : InheritableAttr {
+  let Spellings = [Clang<"os_consumes_this">];
+  let Subjects = SubjectList<[NonStaticCXXMethod]>;
+  let Documentation = [RetainBehaviorDocs];
+}
+
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
   let Args = [FunctionArgument<"FunctionDecl">];
   let Subjects = SubjectList<[LocalVar]>;
   let Documentation = [Undocumented];
 }
 
 def Cold : InheritableAttr {
def NSReturnsAutoreleased : InheritableAttr {
   let Spellings = [Clang<"ns_returns_autoreleased">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumesSelf : InheritableAttr {
   let Spellings = [Clang<"ns_consumes_self">];
   let Subjects = SubjectList<[ObjCMethod]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"ns_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def ObjCException : InheritableAttr {
   let Spellings = [Clang<"objc_exception">];
   let Subjects = SubjectList<[ObjCInterface], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 

include/clang/Basic/AttrDocs.td

 
 It can be overriden using a family of the following attributes.  In
 Objective-C, the annotation ``__attribute__((ns_returns_retained))`` applied to
 a function communicates that the object is returned at ``+1``, and the caller
 is responsible for freeing it.
 Similiarly, the annotation ``__attribute__((ns_returns_not_retained))``
 specifies that the object is returned at ``+0`` and the ownership remains with
 the callee.
+The annotation ``__attribute__((ns_consumes_self))`` specifies that
+the Objective-C method call consumes the reference to ``self``, e.g. by
+attaching it to a supplied parameter.
 Additionally, parameters can have an annotation
 ``__attribute__((ns_consumed))``, which specifies that passing an owned object
 as that parameter effectively transfers the ownership, and the caller is no
 longer responsible for it.
 These attributes affect code generation when interacting with ARC code, and
 they are used by the Clang Static Analyzer.
 
 In C programs using CoreFoundation, a similar set of attributes:
 ``__attribute__((cf_returns_not_retained))``,
 ``__attribute__((cf_returns_retained))`` and ``__attribute__((cf_consumed))``
 have the same respective semantics when applied to CoreFoundation objects.
 These attributes affect code generation when interacting with ARC code, and
 they are used by the Clang Static Analyzer.
 
 Finally, in C++ interacting with XNU kernel (objects inheriting from OSObject),
 the same attribute family is present:
 ``__attribute__((os_returns_not_retained))``,
 ``__attribute__((os_returns_retained))`` and ``__attribute__((os_consumed))``,
 with the same respective semantics.
-These attributes are also used by the Clang Static Analyzer.
+Similar to ``__attribute__((ns_consumes_self))``,
+``__attribute__((os_consumes_this))`` specifies that the method call consumes
+the reference to "this" (e.g., when attaching it to a different object supplied
+as a parameter).
+These attributes are only used by the Clang Static Analyzer.
 
 The family of attributes ``X_returns_X_retained`` can be added to functions,
 C++ methods, and Objective-C methods and properties.
 Attributes ``X_consumed`` can be added to parameters of methods, functions,
 and Objective-C methods.
   }];
 }
 

lib/Sema/SemaDeclAttr.cpp

static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   case ParsedAttr::AT_OSConsumed:
     S.AddXConsumedAttr(D, AL.getRange(), AL.getAttributeSpellingListIndex(),
                      parsedAttrToRetainOwnershipKind(AL),
                      /*IsTemplateInstantiation=*/false);
     break;
   case ParsedAttr::AT_NSConsumesSelf:
     handleSimpleAttribute<NSConsumesSelfAttr>(S, D, AL);
     break;
+  case ParsedAttr::AT_OSConsumesThis:
+    handleSimpleAttribute<OSConsumesThisAttr>(S, D, AL);
+    break;
   case ParsedAttr::AT_NSReturnsAutoreleased:
   case ParsedAttr::AT_NSReturnsNotRetained:
   case ParsedAttr::AT_NSReturnsRetained:
   case ParsedAttr::AT_CFReturnsNotRetained:
   case ParsedAttr::AT_CFReturnsRetained:
   case ParsedAttr::AT_OSReturnsNotRetained:
   case ParsedAttr::AT_OSReturnsRetained:
     handleXReturnsXRetainedAttr(S, D, AL);

test/Sema/attr-osobject.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 struct S {
   __attribute__((os_returns_retained)) S* method_returns_retained() {
     return nullptr;
   }
+
+  __attribute__((os_consumes_this)) void method_consumes_this();
+
+  __attribute__((os_consumes_this)) static void rejected_on_static(); // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
 };
 __attribute__((os_returns_retained)) S *ret_retained() {
   return nullptr;
 }
 
 __attribute__((os_returns_retained)) S ret_retained_value() { // expected-warning{{'os_returns_retained' attribute only applies to functions that return a pointer}}
   return {};
 }

 __attribute__((os_returns_retained(10))) S* returns_retained_no_extra_arg() { // expected-error{{'os_returns_retained' attribute takes no arguments}}
   return nullptr;
 }
 
 struct __attribute__((os_returns_retained)) NoRetainAttrOnStruct {}; // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
 
 __attribute__((os_returns_not_retained(10))) S* os_returns_no_retained_no_extra_args( S *arg) { // expected-error{{'os_returns_not_retained' attribute takes no arguments}}
   return nullptr;
-} 
+}
 
 struct __attribute__((os_returns_not_retained)) NoNotRetainedAttrOnStruct {}; // expected-warning{{'os_returns_not_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
+
+__attribute__((os_consumes_this)) void no_consumes_this_on_function() {} // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}