[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–891

Similarly -> Similar

clang/test/Sema/attr-osobject.cpp

51

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

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

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

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

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

51

But... this is a function?

Also, spurious semi-colon after the function body.

[george.karpenkov]

clang/include/clang/Basic/Attr.td

851

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

852

Yeah I think they should be grouped together.

clang/test/Sema/attr-osobject.cpp

8

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

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

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

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

Still has the spurious semicolon.

[george.karpenkov]

done

[aaron.ballman]

LGTM!

clang/include/clang/Basic/Attr.td

 }
 
 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<[CXXMethod]>;
+  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];
 }
 

clang/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, e.g.
+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.
   }];
 }
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "'overloadable' function %0 must have a prototype">;
 def err_attribute_overloadable_multiple_unmarked_overloads : Error<
   "at most one overload for a given name may lack the 'overloadable' "
   "attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<
   "%0 attribute only applies to %select{functions|methods|properties}1 that "
   "return %select{an Objective-C object|a pointer|a non-retainable pointer}2">,
   InGroup<IgnoredAttributes>;
+def warn_os_consumes_this_static : Warning<
+  "%0 attribute only applies to non-static methods">,
+  InGroup<IgnoredAttributes>;
 def err_ns_attribute_wrong_parameter_type : Error<
   "%0 attribute only applies to "
   "%select{Objective-C object|pointer|pointer-to-CF-pointer}1 parameters">;
 def warn_ns_attribute_wrong_parameter_type : Warning<
   "%0 attribute only applies to "
   "%select{Objective-C object|pointer|pointer-to-CF-pointer}1 parameters">,
   InGroup<IgnoredAttributes>;
 def warn_objc_requires_super_protocol : Warning<

clang/lib/Sema/SemaDeclAttr.cpp

   }
 
   if (A.getKind() == ParsedAttr::AT_AlwaysDestroy)
     handleSimpleAttributeWithExclusions<AlwaysDestroyAttr, NoDestroyAttr>(S, D, A);
   else
     handleSimpleAttributeWithExclusions<NoDestroyAttr, AlwaysDestroyAttr>(S, D, A);
 }
 
+static void handleConsumesThisAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  const auto *MD = cast<CXXMethodDecl>(D);
+  if (MD->isStatic())
+    S.Diag(D->getBeginLoc(), diag::warn_os_consumes_this_static) << AL;
+  handleSimpleAttribute<OSConsumesThisAttr>(S, D, AL);
+}
+
 //===----------------------------------------------------------------------===//
 // Top Level Sema Entry Points
 //===----------------------------------------------------------------------===//
 
 /// ProcessDeclAttribute - Apply the specific attribute to the specified decl if
 /// the attribute applies to decls.  If the attribute is a type attribute, just
 /// silently ignore it if a GNU attribute.
 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:
+    handleConsumesThisAttr(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);

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSplitStack (SubjectMatchRule_function)
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_function)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: OSConsumesThis (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: OSReturnsRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: ObjCBoxable (SubjectMatchRule_record)
 // CHECK-NEXT: ObjCBridge (SubjectMatchRule_record, SubjectMatchRule_type_alias)
 // CHECK-NEXT: ObjCBridgeMutable (SubjectMatchRule_record)
 // CHECK-NEXT: ObjCBridgeRelated (SubjectMatchRule_record)
 // CHECK-NEXT: ObjCException (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCExplicitProtocolImpl (SubjectMatchRule_objc_protocol)

clang/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 static_method_no_consume_this(); // expected-warning{{'os_consumes_this' attribute only applies to non-static methods}}
+
+
+  __attribute__((os_consumes_this(10))) void os_consumes_no_params() {}; // expected-error{{'os_consumes_this' attribute takes no arguments}}
 };
 __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 functions}}
+
+