[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 ↗

(On Diff #180144)

The documentation should be updated to mention this change.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3429 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180590)

Comments can be re-flowed.

434 ↗

(On Diff #180590)

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 ↗

(On Diff #180590)

Comment is incorrect here.

clang/test/Sema/attr-osobject.cpp

52 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180590)

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 ↗

(On Diff #180144)

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 ↗

(On Diff #180144)

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

4860–4861 ↗

(On Diff #180701)

Comments can be re-flowed to 80 col.

4863 ↗

(On Diff #180701)

const auto *

434 ↗

(On Diff #180590)

How about handleSimpleAttributeOrDiagnose()?

clang/test/Sema/attr-osobject.cpp

52 ↗

(On Diff #180144)

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!

cfe/trunk/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 OSReturnsRetainedOnZero : InheritableAttr {
+  let Spellings = [Clang<"os_returns_retained_on_zero">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [RetainBehaviorDocs];
+}
+
+def OSReturnsRetainedOnNonZero : InheritableAttr {
+  let Spellings = [Clang<"os_returns_retained_on_non_zero">];
+  let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def OSConsumesThis : InheritableAttr {
   let Spellings = [Clang<"os_consumes_this">];
   let Subjects = SubjectList<[NonStaticCXXMethod]>;
   let Documentation = [RetainBehaviorDocs];
 }

cfe/trunk/include/clang/Basic/AttrDocs.td

 the same attribute family is present:
 ``__attribute__((os_returns_not_retained))``,
 ``__attribute__((os_returns_retained))`` and ``__attribute__((os_consumed))``,
 with the same respective semantics.
 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).
+Out parameters (parameters the function is meant to write into,
+either via pointers-to-pointers or references-to-pointers)
+may be annotated with ``__attribute__((os_returns_retained))``
+or ``__attribute__((os_returns_not_retained))`` which specifies that the object
+written into the out parameter should (or respectively should not) be released
+after use.
+Since often out parameters may or may not be written depending on the exit
+code of the function,
+annotations ``__attribute__((os_returns_retained_on_zero))``
+and ``__attribute__((os_returns_retained_on_non_zero))`` specify that
+an out parameter at ``+1`` is written if and only if the function returns a zero
+(respectively non-zero) error code.
+Observe that return-code-dependent out parameter annotations are only
+available for retained out parameters, as non-retained object do not have to be
+released by the callee.
 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.
   }];
 }

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

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">;
 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-CF-pointer|pointer/reference-to-OSObject-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">;

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

 template <typename T, typename... DiagnosticArgs>
 static const Sema::SemaDiagnosticBuilder&
 appendDiagnostics(const Sema::SemaDiagnosticBuilder &Bldr, T &&ExtraArg,
                   DiagnosticArgs &&... ExtraArgs) {
   return appendDiagnostics(Bldr << std::forward<T>(ExtraArg),
                            std::forward<DiagnosticArgs>(ExtraArgs)...);
 }
 
-/// Add an attribute {@code AttrType} to declaration {@code D},
-/// provided the given {@code Check} function returns {@code true}
-/// on type of {@code D}.
-/// If check does not pass, emit diagnostic {@code DiagID},
-/// passing in all parameters specified in {@code ExtraArgs}.
+/// Add an attribute {@code AttrType} to declaration {@code D}, provided that
+/// {@code PassesCheck} is true.
+/// Otherwise, emit diagnostic {@code DiagID}, passing in all parameters
+/// specified in {@code ExtraArgs}.
 template <typename AttrType, typename... DiagnosticArgs>
 static void
-handleSimpleAttributeWithCheck(Sema &S, ValueDecl *D, SourceRange SR,
+handleSimpleAttributeOrDiagnose(Sema &S, Decl *D, SourceRange SR,
                                unsigned SpellingIndex,
-                               llvm::function_ref<bool(QualType)> Check,
-                               unsigned DiagID, DiagnosticArgs... ExtraArgs) {
-  if (!Check(D->getType())) {
+                               bool PassesCheck,
+                               unsigned DiagID, DiagnosticArgs&&... ExtraArgs) {
+  if (!PassesCheck) {
     Sema::SemaDiagnosticBuilder DB = S.Diag(D->getBeginLoc(), DiagID);
     appendDiagnostics(DB, std::forward<DiagnosticArgs>(ExtraArgs)...);
     return;
   }
   handleSimpleAttribute<AttrType>(S, D, SR, SpellingIndex);
 }
 
+template <typename AttrType, typename... DiagnosticArgs>
+static void
+handleSimpleAttributeOrDiagnose(Sema &S, Decl *D, const ParsedAttr &AL,
+                               bool PassesCheck,
+                               unsigned DiagID,
+                               DiagnosticArgs&&... ExtraArgs) {
+  return handleSimpleAttributeOrDiagnose<AttrType>(
+      S, D, AL.getRange(), AL.getAttributeSpellingListIndex(), PassesCheck,
+      DiagID, std::forward<DiagnosticArgs>(ExtraArgs)...);
+}
+
 template <typename AttrType>
 static void handleSimpleAttributeWithExclusions(Sema &S, Decl *D,
                                                 const ParsedAttr &AL) {
   handleSimpleAttribute<AttrType>(S, D, AL);
 }
 
 /// Applies the given attribute to the Decl so long as the Decl doesn't
 /// already have one of the given incompatible attributes.

 }
 
 static bool isValidSubjectOfCFAttribute(QualType QT) {
   return QT->isDependentType() || QT->isPointerType() ||
          isValidSubjectOfNSAttribute(QT);
 }
 
 static bool isValidSubjectOfOSAttribute(QualType QT) {
-  return QT->isDependentType() || QT->isPointerType();
+  if (QT->isDependentType())
+    return true;
+  QualType PT = QT->getPointeeType();
+  return !PT.isNull() && PT->getAsCXXRecordDecl() != nullptr;
 }
 
 void Sema::AddXConsumedAttr(Decl *D, SourceRange SR, unsigned SpellingIndex,
                             RetainOwnershipKind K,
                             bool IsTemplateInstantiation) {
   ValueDecl *VD = cast<ValueDecl>(D);
   switch (K) {
   case RetainOwnershipKind::OS:
-    handleSimpleAttributeWithCheck<OSConsumedAttr>(
-        *this, VD, SR, SpellingIndex, &isValidSubjectOfOSAttribute,
+    handleSimpleAttributeOrDiagnose<OSConsumedAttr>(
+        *this, VD, SR, SpellingIndex, isValidSubjectOfOSAttribute(VD->getType()),
         diag::warn_ns_attribute_wrong_parameter_type,
         /*ExtraArgs=*/SR, "os_consumed", /*pointers*/ 1);
     return;
   case RetainOwnershipKind::NS:
-    handleSimpleAttributeWithCheck<NSConsumedAttr>(
-        *this, VD, SR, SpellingIndex, &isValidSubjectOfNSAttribute,
+    handleSimpleAttributeOrDiagnose<NSConsumedAttr>(
+        *this, VD, SR, SpellingIndex, isValidSubjectOfNSAttribute(VD->getType()),
 
         // These attributes are normally just advisory, but in ARC, ns_consumed
         // is significant.  Allow non-dependent code to contain inappropriate
         // attributes even in ARC, but require template instantiations to be
         // set up correctly.
         ((IsTemplateInstantiation && getLangOpts().ObjCAutoRefCount)
              ? diag::err_ns_attribute_wrong_parameter_type
              : diag::warn_ns_attribute_wrong_parameter_type),
         /*ExtraArgs=*/SR, "ns_consumed", /*objc pointers*/ 0);
     return;
   case RetainOwnershipKind::CF:
-    handleSimpleAttributeWithCheck<CFConsumedAttr>(
+    handleSimpleAttributeOrDiagnose<CFConsumedAttr>(
         *this, VD, SR, SpellingIndex,
-        &isValidSubjectOfCFAttribute,
+        isValidSubjectOfCFAttribute(VD->getType()),
         diag::warn_ns_attribute_wrong_parameter_type,
         /*ExtraArgs=*/SR, "cf_consumed", /*pointers*/1);
     return;
   }
 }
 
 static Sema::RetainOwnershipKind
 parsedAttrToRetainOwnershipKind(const ParsedAttr &AL) {
   switch (AL.getKind()) {
   case ParsedAttr::AT_CFConsumed:
+  case ParsedAttr::AT_CFReturnsRetained:
+  case ParsedAttr::AT_CFReturnsNotRetained:
     return Sema::RetainOwnershipKind::CF;
+  case ParsedAttr::AT_OSConsumesThis:
   case ParsedAttr::AT_OSConsumed:
+  case ParsedAttr::AT_OSReturnsRetained:
+  case ParsedAttr::AT_OSReturnsNotRetained:
+  case ParsedAttr::AT_OSReturnsRetainedOnZero:
+  case ParsedAttr::AT_OSReturnsRetainedOnNonZero:
     return Sema::RetainOwnershipKind::OS;
+  case ParsedAttr::AT_NSConsumesSelf:
   case ParsedAttr::AT_NSConsumed:
+  case ParsedAttr::AT_NSReturnsRetained:
+  case ParsedAttr::AT_NSReturnsNotRetained:
+  case ParsedAttr::AT_NSReturnsAutoreleased:
     return Sema::RetainOwnershipKind::NS;
   default:
     llvm_unreachable("Wrong argument supplied");
   }
 }
 
 bool Sema::checkNSReturnsRetainedReturnType(SourceLocation Loc, QualType QT) {
   if (isValidSubjectOfNSReturnsRetainedAttribute(QT))
     return false;
 
   Diag(Loc, diag::warn_ns_attribute_wrong_return_type)
       << "'ns_returns_retained'" << 0 << 0;
   return true;
 }
 
+/// \return whether the parameter is a pointer to OSObject pointer.
+static bool isValidOSObjectOutParameter(const Decl *D) {
+  const auto *PVD = dyn_cast<ParmVarDecl>(D);
+  if (!PVD)
+    return false;
+  QualType QT = PVD->getType();
+  QualType PT = QT->getPointeeType();
+  return !PT.isNull() && isValidSubjectOfOSAttribute(PT);
+}
+
 static void handleXReturnsXRetainedAttr(Sema &S, Decl *D,
                                         const ParsedAttr &AL) {
   QualType ReturnType;
+  Sema::RetainOwnershipKind K = parsedAttrToRetainOwnershipKind(AL);
 
   if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     ReturnType = MD->getReturnType();
   } else if (S.getLangOpts().ObjCAutoRefCount && hasDeclarator(D) &&
              (AL.getKind() == ParsedAttr::AT_NSReturnsRetained)) {
     return; // ignore: was handled as a type attribute
   } else if (const auto *PD = dyn_cast<ObjCPropertyDecl>(D)) {
     ReturnType = PD->getType();
   } 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.
+    unsigned DiagID = K == Sema::RetainOwnershipKind::CF
+            ? /*pointer-to-CF-pointer*/2
+            : /*pointer-to-OSObject-pointer*/3;
     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 << DiagID << 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;
   }
 
   bool TypeOK;
   bool Cf;
+  unsigned ParmDiagID = 2; // Pointer-to-CF-pointer
   switch (AL.getKind()) {
   default: llvm_unreachable("invalid ownership attribute");
   case ParsedAttr::AT_NSReturnsRetained:
     TypeOK = isValidSubjectOfNSReturnsRetainedAttribute(ReturnType);
     Cf = false;
     break;
 
   case ParsedAttr::AT_NSReturnsAutoreleased:
   case ParsedAttr::AT_NSReturnsNotRetained:
     TypeOK = isValidSubjectOfNSAttribute(ReturnType);
     Cf = false;
     break;
 
   case ParsedAttr::AT_CFReturnsRetained:
   case ParsedAttr::AT_CFReturnsNotRetained:
     TypeOK = isValidSubjectOfCFAttribute(ReturnType);
     Cf = true;
     break;
 
   case ParsedAttr::AT_OSReturnsRetained:
   case ParsedAttr::AT_OSReturnsNotRetained:
     TypeOK = isValidSubjectOfOSAttribute(ReturnType);
     Cf = true;
+    ParmDiagID = 3; // Pointer-to-OSObject-pointer
     break;
   }
 
   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 << ParmDiagID << 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))
S.AddXConsumedAttr(D, AL.getRange(), AL.getAttributeSpellingListIndex(),
                      /*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_OSReturnsRetainedOnZero:
+    handleSimpleAttributeOrDiagnose<OSReturnsRetainedOnZeroAttr>(
+        S, D, AL, isValidOSObjectOutParameter(D),
+        diag::warn_ns_attribute_wrong_parameter_type,
+        /*Extra Args=*/AL, /*pointer-to-OSObject-pointer*/ 3, AL.getRange());
+    break;
+  case ParsedAttr::AT_OSReturnsRetainedOnNonZero:
+    handleSimpleAttributeOrDiagnose<OSReturnsRetainedOnNonZeroAttr>(
+        S, D, AL, isValidOSObjectOutParameter(D),
+        diag::warn_ns_attribute_wrong_parameter_type,
+        /*Extra Args=*/AL, /*pointer-to-OSObject-poointer*/ 3, AL.getRange());
+    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);

cfe/trunk/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: OSReturnsRetainedOnNonZero (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: OSReturnsRetainedOnZero (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: ObjCExternallyRetained (SubjectMatchRule_variable_not_is_parameter, SubjectMatchRule_function, SubjectMatchRule_block, SubjectMatchRule_objc_method)
 // CHECK-NEXT: ObjCMethodFamily (SubjectMatchRule_objc_method)

cfe/trunk/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) {}
+
+bool write_into_out_parameter_on_nonzero(__attribute__((os_returns_retained_on_non_zero)) S** out) {}
+
+bool write_into_out_parameter_on_nonzero_invalid(__attribute__((os_returns_retained_on_non_zero)) S* out) {} // expected-warning{{'os_returns_retained_on_non_zero' attribute only applies to pointer/reference-to-OSObject-pointer parameters}}
+
+bool write_into_out_parameter_on_zero(__attribute__((os_returns_retained_on_zero)) S** out) {}
+
+bool write_into_out_parameter_on_zero_invalid(__attribute__((os_returns_retained_on_zero)) S* out) {} // expected-warning{{'os_returns_retained_on_zero' attribute only applies to pointer/reference-to-OSObject-pointer parameters}}
+
+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/reference-to-OSObject-pointer parameters}}
+
+void write_into_out_parameter_not_retained(__attribute__((os_returns_not_retained)) S **out) {}
+
+void write_into_out_parameter_not_retained(__attribute__((os_returns_not_retained)) S volatile * const * const 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/reference-to-OSObject-pointer parameters}}
+
+void write_into_not_retainedout_parameter_too_many_pointers(__attribute__((os_returns_not_retained)) S*** out) {} // expected-warning{{'os_returns_not_retained' attribute only applies to pointer/reference-to-OSObject-pointer parameters}}

cfe/trunk/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;
 };