[attributes] Extend os_returns_(not_?)_retained attributes to parameters.

[george.karpenkov]

Summary

When applied to out-parameters, the attributes specify the expected lifetime of the written-into object.

Additionally, introduce OSReturnsRetainedOn(Non)Zero attributes, which specify that an ownership transfer happens depending on a return code.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

844

The documentation should be updated to mention this change.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3429

Do we have no semantic tests that verified the pointer-to-CF-pointer case was actually a pointer to a CF pointer?

clang/test/Sema/attr-osobject.cpp

52

This is not a pointer-to-pointer parameter. I assume the behavior here is correct, but it suggests that in C++ mode the diagnostic should say "pointer- or reference-to-pointer", perhaps. This should also be called out in the docs, I think.

60

Can you also add some test cases for qualified pointers? e.g., should this work?

void write_into_out_parameter_not_retained(__attribute__((os_returns_not_retained)) S volatile * restrict * const out) {}

[george.karpenkov]

Addressed comments, added conditional out parameters.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3429

Fair point.
We do have such a test. I was too lazy too add another case to diagnostics, as it's a bit of extra code. But maybe we should do that.

clang/test/Sema/attr-osobject.cpp

52

Sorry I don't quite agree here: while it's technically not pointer to pointer, the semantics is nearly identical (save for null parameters).

I think it's good to be flexible enough to accept those, without unnecessarily complicating the documentation or diagnostics.

60

I don't know what "restrict" is, and it does not compile, but added others.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

899 ↗

(On Diff #180590)

I'd spell out "iff" if you really meant "if and only if".

902 ↗

(On Diff #180590)

non-retained objects

clang/include/clang/Basic/DiagnosticSemaKinds.td

3429

Should it be pointer-to-OS-pointer, or perhaps pointer-to-CFObject-pointer? Or do we not need to be consistent with nomenclature that way?

clang/lib/Sema/SemaDeclAttr.cpp

429–430

Comments can be re-flowed.

434

I think the function name is a bit of a misnomer now because there's no real check being performed within the call -- the caller does the checking. Perhaps handleSimpleAttributeWithDiagnostic() instead, and the parameter can be named ShouldDiagnose?

4920

Comment is incorrect here.

clang/test/Sema/attr-osobject.cpp

52

I think it's good to be flexible enough to accept those, without unnecessarily complicating the documentation or diagnostics.

I agree that it seems reasonable to accept references because the semantics are so similar to those of pointers, but I disagree that being precise with the documentation and diagnostics is unnecessary complication. When the diagnostic says "this attribute only applies to", users may reasonably expect that "only" is meaningful and miss out on the fact that there's an even better, supported construct for them to use when C++ language features are enabled.

I thought of one more test case we don't have coverage for (it's not super specific to your patch, but you do touch on it). Can you add:

bool two_stars_good_three_stars_bad(__attribute__((os_returns_retained_on_zero)) S*** out) {}

I assume that should diagnose because it's a pointer to a pointer to a pointer to an OS object.

60

It's a C99 qualifier: https://godbolt.org/z/jz2im5, but it's not terribly important for the test either. That said, I don't see the new test case added?

[george.karpenkov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3429

No, here it does not need to be consistent: CF is an accepted unambiguous abbreviation if you are interacting with Apple frameworks, and OS is not.

clang/lib/Sema/SemaDeclAttr.cpp

434

I agree that the name is not perfect, but I think handleSimpleAttributeWithDiagnostic
with ShouldDiagnose is worse, because (to me) it implies additive action: we handle the attribute, and then we potentially emit diagnostics.
Whereas the actual semantics is: we handle the attribute if it passes the test,
and emit the diagnostics otherwise. But encoding that seems to verbose.

clang/test/Sema/attr-osobject.cpp

52

I assume that should diagnose because it's a pointer to a pointer to a pointer to an OS object.

Actually we don't diagnose it, but perhaps you are right and we should.

miss out on the fact that there's an even better, supported construct for them to use when C++ language features are enabled.

OK. It does get more verbose though, which IMO can be bad for diagnostics (e.g. in my setup clangd shows warnings and errors in the vim statusbar, and it gets cut if it does not fit. I imagine many are like that)

60

Ooops, sorry, must have lost it.

[george.karpenkov]

@aaron.ballman I'm half-tempted to rewrite most of attribute handling to just have a single attribute class with multiple spelling, and then I would be able to differentiate between them without meta-programming template trickery.
That poses the problem though: dumping both parameter and function/method (actually, the latter two together are ugly already) into the same attribute class would make the code quite a bit less declarative, and would require writing quite a bit of imperative logic to distinguish between the two.
Dumping them into two buckets (for parameters and for callables) would have a problem of not having a nice way to share an enum between the two (c.f Ownership::OwnershipKind in Attr.td. Or maybe it's OK for one attribute to refer to an enum inside the other?)

What do you think?

[george.karpenkov]

OTOH using a single attribute class for all attributes makes usage from Sema very unwieldy,
as then it would not be possible to write D->hasAttr<BlahAttr> anymore =/

[aaron.ballman]

LGTM aside from some minor nits.

clang/lib/Sema/SemaDeclAttr.cpp

434

How about handleSimpleAttributeOrDiagnose()?

4860–4861

Comments can be re-flowed to 80 col.

4863

const auto *

clang/test/Sema/attr-osobject.cpp

52

OK. It does get more verbose though, which IMO can be bad for diagnostics (e.g. in my setup clangd shows warnings and errors in the vim statusbar, and it gets cut if it does not fit. I imagine many are like that)

So long as the diagnostic reasonably continues to fit in a single sentence fragment, I think it's "short enough" -- I don't think we should leave useful information out of diagnostics because some editors have poor Ux for displaying longer diagnostic messages. I don't think we've reached that tipping point here yet, so thank you for the change!

clang/include/clang/Basic/Attr.td

 def OSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"os_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSReturnsRetained : InheritableAttr {
   let Spellings = [Clang<"os_returns_retained">];
-  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
+  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"os_returns_not_retained">];
-  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
+  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty, ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSConsumesThis : InheritableAttr {
   let Spellings = [Clang<"os_consumes_this">];
   let Subjects = SubjectList<[NonStaticCXXMethod]>;
   let Documentation = [RetainBehaviorDocs];
 }

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "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 err_ns_attribute_wrong_parameter_type : Error<
   "%0 attribute only applies to "
-  "%select{Objective-C object|pointer|pointer-to-CF-pointer}1 parameters">;
+  "%select{Objective-C object|pointer|pointer-to-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">,
+  "%select{Objective-C object|pointer|pointer-to-pointer}1 parameters">,
   InGroup<IgnoredAttributes>;
 def warn_objc_requires_super_protocol : Warning<
   "%0 attribute cannot be applied to %select{methods in protocols|dealloc}1">,
   InGroup<DiagGroup<"requires-super-attribute">>;
 def note_protocol_decl : Note<
   "protocol is declared here">;
 def note_protocol_decl_undefined : Note<
   "protocol %0 has no definition">;

clang/lib/Sema/SemaDeclAttr.cpp

   } else if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     ReturnType = FD->getReturnType();
   } else if (const auto *Param = dyn_cast<ParmVarDecl>(D)) {
     // Attributes on parameters are used for out-parameters,
     // passed as pointers-to-pointers.
     ReturnType = Param->getType()->getPointeeType();
     if (ReturnType.isNull()) {
       S.Diag(D->getBeginLoc(), diag::warn_ns_attribute_wrong_parameter_type)
-          << AL << /*pointer-to-CF-pointer*/ 2 << AL.getRange();
+          << AL << /*pointer-to-pointer*/ 2 << AL.getRange();
       return;
     }
   } else if (AL.isUsedAsTypeAttr()) {
     return;
   } else {
     AttributeDeclKind ExpectedDeclKind;
     switch (AL.getKind()) {
     default: llvm_unreachable("invalid ownership attribute");
     case ParsedAttr::AT_NSReturnsRetained:
     case ParsedAttr::AT_NSReturnsAutoreleased:
     case ParsedAttr::AT_NSReturnsNotRetained:
-    case ParsedAttr::AT_OSReturnsRetained:
-    case ParsedAttr::AT_OSReturnsNotRetained:
       ExpectedDeclKind = ExpectedFunctionOrMethod;
       break;
 
+    case ParsedAttr::AT_OSReturnsRetained:
+    case ParsedAttr::AT_OSReturnsNotRetained:
     case ParsedAttr::AT_CFReturnsRetained:
     case ParsedAttr::AT_CFReturnsNotRetained:
       ExpectedDeclKind = ExpectedFunctionMethodOrParameter;
       break;
     }
     S.Diag(D->getBeginLoc(), diag::warn_attribute_wrong_decl_type)
         << AL.getRange() << AL << ExpectedDeclKind;
     return;

   }
 
   if (!TypeOK) {
     if (AL.isUsedAsTypeAttr())
       return;
 
     if (isa<ParmVarDecl>(D)) {
       S.Diag(D->getBeginLoc(), diag::warn_ns_attribute_wrong_parameter_type)
-          << AL << /*pointer-to-CF*/ 2 << AL.getRange();
+          << AL << /*pointer-to-pointer*/ 2 << AL.getRange();
     } else {
       // Needs to be kept in sync with warn_ns_attribute_wrong_return_type.
       enum : unsigned {
         Function,
         Method,
         Property
       } SubjectKind = Function;
       if (isa<ObjCMethodDecl>(D))
         SubjectKind = Method;
       else if (isa<ObjCPropertyDecl>(D))
         SubjectKind = Property;
       S.Diag(D->getBeginLoc(), diag::warn_ns_attribute_wrong_return_type)
           << AL << SubjectKind << Cf << AL.getRange();
     }
     return;
   }
 
   switch (AL.getKind()) {
     default:

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: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
-// CHECK-NEXT: OSReturnsRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
+// CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: OSReturnsRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
 // 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)
 // CHECK-NEXT: ObjCMethodFamily (SubjectMatchRule_objc_method)
 // CHECK-NEXT: ObjCPreciseLifetime (SubjectMatchRule_variable)

clang/test/Sema/attr-osobject.cpp

 void accept_consumed_arg_no_extra_arg(__attribute__((os_consumed(10))) S *arg) {} // expected-error{{'os_consumed' attribute takes no arguments}}
 
 struct __attribute__((os_consumed)) NoAttrOnStruct {}; // expected-warning{{'os_consumed' attribute only applies to parameters}}
 
 __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}}
+struct __attribute__((os_returns_retained)) NoRetainAttrOnStruct {}; // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, Objective-C properties, and parameters}}
 
 __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}}
+struct __attribute__((os_returns_not_retained)) NoNotRetainedAttrOnStruct {}; // expected-warning{{'os_returns_not_retained' attribute only applies to functions, Objective-C methods, Objective-C properties, and parameters}}
 
 __attribute__((os_consumes_this)) void no_consumes_this_on_function() {} // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
+
+void write_into_out_parameter(__attribute__((os_returns_retained)) S** out) {}
+
+void write_into_out_parameter_ref(__attribute__((os_returns_retained)) S*& out) {}
+
+typedef S* SPtr;
+
+void write_into_out_parameter_typedef(__attribute__((os_returns_retained)) SPtr* out) {}
+
+void write_into_out_parameter_invalid(__attribute__((os_returns_retained)) S* out) {} // expected-warning{{'os_returns_retained' attribute only applies to pointer-to-pointer parameters}}
+
+void write_into_out_parameter_not_retained(__attribute__((os_returns_not_retained)) S **out) {}
+
+void write_into_not_retainedout_parameter_invalid(__attribute__((os_returns_not_retained)) S* out) {} // expected-warning{{'os_returns_not_retained' attribute only applies to pointer-to-pointer parameters}}

clang/test/Sema/attr-osobject.mm

 // RUN: %clang_cc1 -fsyntax-only -verify -fblocks %s
 
 struct S {};
 
 @interface I
   @property (readonly) S* prop __attribute__((os_returns_retained));
   - (S*) generateS __attribute__((os_returns_retained));
   - (void) takeS:(S*) __attribute__((os_consumed)) s;
 @end
 
-typedef __attribute__((os_returns_retained)) id (^blockType)(); // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
+typedef __attribute__((os_returns_retained)) id (^blockType)(); // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, Objective-C properties, and parameters}}
 
-__auto_type b = ^ id (id filter)  __attribute__((os_returns_retained))  { // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
+__auto_type b = ^ id (id filter)  __attribute__((os_returns_retained))  { // expected-warning{{'os_returns_retained' attribute only applies to functions, Objective-C methods, Objective-C properties, and parameters}}
   return filter;
 };

clang/test/SemaObjC/attr-cf_returns.m

 
 @property int invalidProp1 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to properties that return a pointer}}
 @property void invalidProp2 CF_RETURNS_RETAINED; // expected-warning{{'cf_returns_retained' attribute only applies to properties that return a pointer}}
 
 @property CFFooRef valid1 CF_RETURNS_RETAINED;
 @property id valid2 CF_RETURNS_RETAINED;
 @end
 
-void invalidParam(int a CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}}
-                  int *b CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}}
-                  id c CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}}
-                  void *d CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}}
-                  CFFooRef e CF_RETURNS_RETAINED); // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-CF-pointer parameters}}
+void invalidParam(int a CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-pointer parameters}}
+                  int *b CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-pointer parameters}}
+                  id c CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-pointer parameters}}
+                  void *d CF_RETURNS_RETAINED, // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-pointer parameters}}
+                  CFFooRef e CF_RETURNS_RETAINED); // expected-warning{{'cf_returns_retained' attribute only applies to pointer-to-pointer parameters}}
 
 void validParam(id *a CF_RETURNS_RETAINED,
                 CFFooRef *b CF_RETURNS_RETAINED,
                 void **c CF_RETURNS_RETAINED);
 void validParam2(id *a CF_RETURNS_NOT_RETAINED,
                  CFFooRef *b CF_RETURNS_NOT_RETAINED,
                  void **c CF_RETURNS_NOT_RETAINED);