[attributes] Add a family of OS_CONSUMED, OS_RETURNS and OS_RETURNS_RETAINED attributes

[george.karpenkov]

Summary

The addition adds three attributes for communicating ownership,
analogous to existing NS_ and CF_ attributes.
The attributes are meant to be used for communicating ownership of all objects in XNU (Darwin kernel) and all of the kernel modules.
The ownership model there is very similar, but still different from the Foundation model, so we think that introducing a new family of attributes is appropriate.

The addition required a sizeable refactoring of the existing code for CF_ and NS_ ownership attributes,
due to tight coupling and the fact that differentiating between the types was previously done using a boolean.

[george.karpenkov]

@aaron.ballman Any major objections?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

834 ↗

(On Diff #175311)

No new undocumented attributes, please. (Same for the other attributes.)

837 ↗

(On Diff #175311)

Are there no subjects for this attribute?

842 ↗

(On Diff #175311)

Or this one?

clang/lib/Sema/SemaDeclAttr.cpp

395–400 ↗

(On Diff #175311)

Did clang-format do this?

417 ↗

(On Diff #175311)

T &&ExtraArg and std::forward it below?

418 ↗

(On Diff #175311)

Missing && here, no?

430 ↗

(On Diff #175311)

I don't see a strong relationship between this function name and the fact that the check function is doing something with type checking. The function sounds more generic than it really is.

434 ↗

(On Diff #175311)

I don't like this being a cast<> here. I would do: if (auto *VD = dyn_cast<ValueDecl>(D)) { ... } to avoid the assertion.

436 ↗

(On Diff #175311)

Is this top-level const because you planned to assign to a const reference, or do you really want the copy here? If you want the copy, please drop the const.

4851 ↗

(On Diff #175311)

Spurious newline.

clang/test/SemaTemplate/attributes.cpp

57–76 ↗

(On Diff #175311)

These should be in a test in Sema, not SemaTemplate (they're not related to template instantiation).

[george.karpenkov]

clang/include/clang/Basic/Attr.td

834 ↗

(On Diff #175311)

OK

837 ↗

(On Diff #175311)

I've copied it from the other family of attributes; TBH I'm not sure what the field is doing. I'll see whether I can come up with a good value.

842 ↗

(On Diff #175311)

OK, same as above --- I'll try to put something reasonable there.

clang/lib/Sema/SemaDeclAttr.cpp

395–400 ↗

(On Diff #175311)

It's a different function from the previous one -- I have introduced another handleSimpleAttribute which takes a SourceRange and a SpellingIndex directly, and now the previous one calls it.

417 ↗

(On Diff #175311)

OK

430 ↗

(On Diff #175311)

Ideally it should really take a check on a Decl, and not a type,
but in this case it led to unnecessary code duplication (as in one of the use cases only type, and not the decl is available).
I think it's quite generic - it performs and check on the declaration type.

In such situations I usually favor the YAGNI principle - leave it like this, and if someone needs a check on a decl, and not an attribute, this function could easily be generalized.

434 ↗

(On Diff #175311)

OK. In that case being not a value decl would implicitly fial the diagnostic check.

436 ↗

(On Diff #175311)

I dislike having references to returned values, as RVO would avoid the copy anyway.

Conceptually, it should be a reference, but in this case we don't need it (due to RVO),
and having an extra copy before any diagnostic is emitted won't hurt.

Later on it is passed as a parameter to a function accepting a const reference.

4851 ↗

(On Diff #175311)

I like to follow Google style guide with a newline preceding each comment, but OK.

clang/test/SemaTemplate/attributes.cpp

57–76 ↗

(On Diff #175311)

OK

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1634 ↗

(On Diff #175593)

Should this also point to RetainBehaviorDocs now?

837 ↗

(On Diff #175311)

The subject list specifies what kind of entities the attribute can appertain to; it takes care of some of the automated testing for attributes so you don't have to do the work manually.

clang/include/clang/Basic/AttrDocs.td

860 ↗

(On Diff #175593)

You can re-flow the documentation for 80-col.

882–883 ↗

(On Diff #175593)

These are probably meant to be os_* and not cf_*.

clang/lib/Sema/SemaDeclAttr.cpp

395–400 ↗

(On Diff #175311)

My comment was more about the formatting. I saw that this is a new overload, but the old formatting was very strange. It looks correct now.

430 ↗

(On Diff #175311)

I think it's quite generic - it performs and check on the declaration type.

Not all declarations have types. For instance, you can apply an attribute to a namespace declaration (such as [[deprecated]]). So my concern is more that this function isn't actually generic when it sounds like it is.

In such situations I usually favor the YAGNI principle - leave it like this, and if someone needs a check on a decl, and not an attribute, this function could easily be generalized.

Yeah, I don't think we need to change the behavior of the function, but I am wondering if it makes sense to rename it to handleSimpleAttributeWithTypeCheck() or some-such to make it more clear that the attribute must appertain to a declaration with a type. Alternatively, instead of taking a Decl *, it could accept a ValueDecl * instead and force the user to only pass in declarations with types to the call.

436 ↗

(On Diff #175311)

I'm reluctantly okay with using value semantics here (I have a preference for using const reference semantics instead so we can avoid a needless local copy), but the top-level const should go in that case (we don't typically use that idiom in the project). I was only asking because I could not tell if that was a typo for const Sema::SemaDiagnosticBuilder & or not.

clang/test/Sema/attr-osobject.cpp

1 ↗

(On Diff #175593)

This doesn't need to pass -std=c++11.

4 ↗

(On Diff #175593)

Can you run this file through clang-format?

22 ↗

(On Diff #175593)

You should also add tests that ensure that the attribute does not accept arguments and doesn't appertain to things that are not functions.

Related Q: should all of these attributes also appertain to function pointers? (This may be orthogonal to this patch.)

[george.karpenkov]

clang/lib/Sema/SemaDeclAttr.cpp

430 ↗

(On Diff #175311)

OK, let's accept a ValueDecl.

436 ↗

(On Diff #175311)

Dropping const.
I don't think we should have a reference here, as then there is no clear owner for the builder, and it becomes easy to end up with a dead reference.

clang/test/Sema/attr-osobject.cpp

22 ↗

(On Diff #175593)

Attributes should only apply to pointers to objects (inside the analyzer it's even more restricted: only pointers to objects inheriting from OSObject).

[aaron.ballman]

clang/include/clang/Basic/Attr.td

839 ↗

(On Diff #175602)

The CFReturnsRetained (and NotRetained) use a commented-out subject list that includes ObjCProperty. Do the OS functions also apply to ObjCProperty objects? I believe I added the above commented out because we couldn't automate something about it, and I vaguely recall the subject list was my best effort at gleaning the subjects (so it might be inaccurate).

clang/lib/Sema/SemaDeclAttr.cpp

4773–4778 ↗

(On Diff #175602)

This doesn't look formatted properly to me. Did clang-format do this?

4789 ↗

(On Diff #175602)

Can remove spurious newline.

clang/test/Sema/attr-osobject.cpp

22 ↗

(On Diff #175593)

Do the same kind of tests for os_returns_retained and os_returns_not_retained.

You should also add an Objective-C test for their methods and properties (those tests can be positive tests showing that no diagnostics are expected).

[george.karpenkov]

clang/include/clang/Basic/Attr.td

839 ↗

(On Diff #175602)

No, they shouldn't apply to properties.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

839 ↗

(On Diff #175602)

handleXReturnsXRetainedAttr() suggests otherwise for the other attributes, so is there a reason this one is different?

For instance: https://godbolt.org/z/stN5rO

clang/lib/Sema/SemaDeclAttr.cpp

4838 ↗

(On Diff #175726)

This seems to imply that an ObjCPropertyDecl is acceptable to all of the returns_*_retained attributes (was there before your refactoring).

4862 ↗

(On Diff #175726)

And then this suggests that NS and OS do not accept properties (similar below for CF).

If NS and CF are expected to work on properties, we should probably update this diagnostic (doesn't have to be part of this patch). If they aren't supposed to work on properties, then we should fix the logic.

[george.karpenkov]

clang/include/clang/Basic/Attr.td

839 ↗

(On Diff #175602)

Hm, that's an interesting point, thanks for bringing this up.

On one hand, adding OS_RETURNS_RETAINED to properties makes no sense - the semantics of the program is really weird if accessing the property transfers the ownership.
But on the other hand it's explicitly allowed and tested in attr-cf_returns on CF_* properties, so we should probably allow it as well.

clang/lib/Sema/SemaDeclAttr.cpp

4862 ↗

(On Diff #175726)

They do work, and it's actually explicitly tested. I think for the purpose of symmetry we should also support it for OS_* attributes then.

Since a lot of code relies on behavior of NS_* properties for code generation, I'm not going to change the logic for that behavior, unless the actual need arises.
FWIW the error message is actually correct when the attribute is added to the property of the unexpected type.

[george.karpenkov]

@aaron.ballman Thanks for your input!

[aaron.ballman]

LGTM aside from a small documentation nit.

clang/include/clang/Basic/AttrDocs.td

853 ↗

(On Diff #175731)

You should add a quick note somewhere in here that some of these attributes appertains to functions, Objective-C methods, and Objective-C properties.

clang/lib/Sema/SemaDeclAttr.cpp

4862 ↗

(On Diff #175726)

FWIW the error message is actually correct when the attribute is added to the property of the unexpected type.

I think it's incorrect -- that error message results in this, which doesn't mention ObjC properties: https://godbolt.org/z/UZHHN_

I can fix this up in a separate patch, however.

[aaron.ballman]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

I mentioned that I could fix this up in another patch, but there's a different subtlety here -- you can apply NSReturnsRetained and friends to types. See test\SemaObjC\attr-ns_returns_retained.m for an example -- where it's being added to a block type. It also seems to appertain to block literals (test\SemaObjC\block-literal-with-attribute.m).

Should the OS versions also apply to types and block literals?

[george.karpenkov]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

OS versions should not apply to block literals, neither they should be applicable to block types.

[aaron.ballman]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

Can you add some tests to ensure it's properly diagnosed?

[george.karpenkov]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

@aaron.ballman Do you mean something like this?

diff --git a/clang/test/Sema/attr-osobject.mm b/clang/test/Sema/attr-osobject.mm
index dbd9122a8fa..ffb4394e4bc 100644
--- a/clang/test/Sema/attr-osobject.mm
+++ b/clang/test/Sema/attr-osobject.mm
@@ -1,6 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
-
-// expected-no-diagnostics
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s

 struct S {};

@@ -9,3 +7,5 @@ @interface I
   - (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}}

[aaron.ballman]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

Yup! Also:

__auto_type b = ^ id (id filter)  __attribute__((os_returns_retained))  {
  return filter;
};

This is accepted by ns_returns_retained, so it would be good to clearly show it's not supported by os_returns_retained.

[george.karpenkov]

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

4862

@aaron.ballman committed in rL348443

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

def CFUnknownTransfer : InheritableAttr {
   let Spellings = [Clang<"cf_unknown_transfer">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def CFReturnsRetained : InheritableAttr {
   let Spellings = [Clang<"cf_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def CFReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"cf_returns_not_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def CFConsumed : InheritableParamAttr {
   let Spellings = [Clang<"cf_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
+}
+
+// OSObject-based attributes.
+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 Documentation = [RetainBehaviorDocs];
+}
+
+def OSReturnsNotRetained : InheritableAttr {
+  let Spellings = [Clang<"os_returns_not_retained">];
+  let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
   let Args = [FunctionArgument<"FunctionDecl">];
   let Subjects = SubjectList<[LocalVar]>;
   let Documentation = [Undocumented];
 }
let Args = [IdentifierArgument<"RelatedClass">,
           IdentifierArgument<"InstanceMethod">];
   let HasCustomParsing = 1;
   let Documentation = [Undocumented];
 }
 
 def NSReturnsRetained : DeclOrTypeAttr {
   let Spellings = [Clang<"ns_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"ns_returns_not_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSReturnsAutoreleased : InheritableAttr {
   let Spellings = [Clang<"ns_returns_autoreleased">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumesSelf : InheritableAttr {
   let Spellings = [Clang<"ns_consumes_self">];
   let Subjects = SubjectList<[ObjCMethod]>;
   let Documentation = [Undocumented];
 }
 

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

 merely want to document its non-standard retain and release semantics, should
 use the retaining behavior attributes (``ns_returns_retained``,
 ``ns_returns_not_retained``, etc).
 
 Query for this feature with ``__has_attribute(objc_method_family)``.
   }];
 }
 
+def RetainBehaviorDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The behavior of a function with respect to reference counting for Foundation
+(Objective-C), CoreFoundation (C) and OSObject (C++) is determined by a naming
+convention (e.g. functions starting with "get" are assumed to return at
+``+0``).
+
+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.
+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.
+
+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.
+  }];
+}
+
+
+
 def NoDebugDocs : Documentation {
   let Category = DocCatVariable;
   let Content = [{
 The ``nodebug`` attribute allows you to suppress debugging information for a
 function or method, or for a variable that is not a parameter or a non-static
 data member.
   }];
 }

cfe/trunk/include/clang/Sema/Sema.h

 
   /// AddModeAttr - Adds a mode attribute to a particular declaration.
   void AddModeAttr(SourceRange AttrRange, Decl *D, IdentifierInfo *Name,
                    unsigned SpellingListIndex, bool InInstantiation = false);
 
   void AddParameterABIAttr(SourceRange AttrRange, Decl *D,
                            ParameterABI ABI, unsigned SpellingListIndex);
 
-  void AddNSConsumedAttr(SourceRange AttrRange, Decl *D,
-                         unsigned SpellingListIndex, bool isNSConsumed,
-                         bool isTemplateInstantiation);
+  enum class RetainOwnershipKind {NS, CF, OS};
+  void AddXConsumedAttr(Decl *D, SourceRange SR, unsigned SpellingIndex,
+                        RetainOwnershipKind K, bool IsTemplateInstantiation);
 
   bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);
 
   //===--------------------------------------------------------------------===//
   // C++ Coroutines TS
   //
   bool ActOnCoroutineBodyStart(Scope *S, SourceLocation KwLoc,
                                StringRef Keyword);

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

bool Sema::checkStringLiteralArgumentAttr(const ParsedAttr &AL, unsigned ArgNum,
 
   Str = Literal->getString();
   return true;
 }
 
 /// Applies the given attribute to the Decl without performing any
 /// additional semantic checking.
 template <typename AttrType>
+static void handleSimpleAttribute(Sema &S, Decl *D, SourceRange SR,
+                                  unsigned SpellingIndex) {
+  D->addAttr(::new (S.Context) AttrType(SR, S.Context, SpellingIndex));
+}
+
+template <typename AttrType>
 static void handleSimpleAttribute(Sema &S, Decl *D, const ParsedAttr &AL) {
-  D->addAttr(::new (S.Context) AttrType(AL.getRange(), S.Context,
-                                        AL.getAttributeSpellingListIndex()));
+  handleSimpleAttribute<AttrType>(S, D, AL.getRange(),
+                                  AL.getAttributeSpellingListIndex());
+}
+
+
+template <typename... DiagnosticArgs>
+static const Sema::SemaDiagnosticBuilder&
+appendDiagnostics(const Sema::SemaDiagnosticBuilder &Bldr) {
+  return Bldr;
+}
+
+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}.
+template <typename AttrType, typename... DiagnosticArgs>
+static void
+handleSimpleAttributeWithCheck(Sema &S, ValueDecl *D, SourceRange SR,
+                               unsigned SpellingIndex,
+                               llvm::function_ref<bool(QualType)> Check,
+                               unsigned DiagID, DiagnosticArgs... ExtraArgs) {
+  if (!Check(D->getType())) {
+    Sema::SemaDiagnosticBuilder DB = S.Diag(D->getBeginLoc(), DiagID);
+    appendDiagnostics(DB, std::forward<DiagnosticArgs>(ExtraArgs)...);
+    return;
+  }
+  handleSimpleAttribute<AttrType>(S, D, SR, SpellingIndex);
 }
 
 template <typename AttrType>
 static void handleSimpleAttributeWithExclusions(Sema &S, Decl *D,
                                                 const ParsedAttr &AL) {
   handleSimpleAttribute<AttrType>(S, D, AL);
 }
 
static void handleXRayLogArgsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(::new (S.Context) XRayLogArgsAttr(
       AL.getRange(), S.Context, ArgCount.getSourceIndex(),
       AL.getAttributeSpellingListIndex()));
 }
 
 //===----------------------------------------------------------------------===//
 // Checker-specific attribute handlers.
 //===----------------------------------------------------------------------===//
-
 static bool isValidSubjectOfNSReturnsRetainedAttribute(QualType QT) {
   return QT->isDependentType() || QT->isObjCRetainableType();
 }
 
-static bool isValidSubjectOfNSAttribute(Sema &S, QualType QT) {
+static bool isValidSubjectOfNSAttribute(QualType QT) {
   return QT->isDependentType() || QT->isObjCObjectPointerType() ||
-         S.Context.isObjCNSObjectType(QT);
+         QT->isObjCNSObjectType();
 }
 
-static bool isValidSubjectOfCFAttribute(Sema &S, QualType QT) {
+static bool isValidSubjectOfCFAttribute(QualType QT) {
   return QT->isDependentType() || QT->isPointerType() ||
-         isValidSubjectOfNSAttribute(S, QT);
+         isValidSubjectOfNSAttribute(QT);
 }
 
-static void handleNSConsumedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  S.AddNSConsumedAttr(AL.getRange(), D, AL.getAttributeSpellingListIndex(),
-                      AL.getKind() == ParsedAttr::AT_NSConsumed,
-                      /*template instantiation*/ false);
+static bool isValidSubjectOfOSAttribute(QualType QT) {
+  return QT->isDependentType() || QT->isPointerType();
 }
 
-void Sema::AddNSConsumedAttr(SourceRange AttrRange, Decl *D,
-                             unsigned SpellingIndex, bool IsNSConsumed,
-                             bool IsTemplateInstantiation) {
-  const auto *Param = cast<ParmVarDecl>(D);
-  bool TypeOK;
-
-  if (IsNSConsumed)
-    TypeOK = isValidSubjectOfNSAttribute(*this, Param->getType());
-  else
-    TypeOK = isValidSubjectOfCFAttribute(*this, Param->getType());
+void Sema::AddXConsumedAttr(Decl *D, SourceRange SR, unsigned SpellingIndex,
+                            RetainOwnershipKind K,
+  ValueDecl *VD = cast<ValueDecl>(D);
+  switch (K) {
+  case RetainOwnershipKind::OS:
+    handleSimpleAttributeWithCheck<OSConsumedAttr>(
+        *this, VD, SR, SpellingIndex, &isValidSubjectOfOSAttribute,
+        diag::warn_ns_attribute_wrong_parameter_type,
+        /*ExtraArgs=*/SR, "os_consumed", /*pointers*/ 1);
+    return;
+  case RetainOwnershipKind::NS:
+    handleSimpleAttributeWithCheck<NSConsumedAttr>(
+        *this, VD, SR, SpellingIndex, &isValidSubjectOfNSAttribute,
 
-  if (!TypeOK) {
         // 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.
-    Diag(D->getBeginLoc(), (IsTemplateInstantiation && IsNSConsumed &&
-                                    getLangOpts().ObjCAutoRefCount
-                                ? diag::err_ns_attribute_wrong_parameter_type
-                                : diag::warn_ns_attribute_wrong_parameter_type))
-        << AttrRange << (IsNSConsumed ? "ns_consumed" : "cf_consumed")
-        << (IsNSConsumed ? /*objc pointers*/ 0 : /*cf pointers*/ 1);
+        ((IsTemplateInstantiation && getLangOpts().ObjCAutoRefCount)
+             : diag::warn_ns_attribute_wrong_parameter_type),
+        /*ExtraArgs=*/SR, "ns_consumed", /*objc pointers*/ 0);
     return;
-  }
+  case RetainOwnershipKind::CF:
+    handleSimpleAttributeWithCheck<CFConsumedAttr>(
+        *this, VD, SR, SpellingIndex,
+        &isValidSubjectOfCFAttribute,
+        diag::warn_ns_attribute_wrong_parameter_type,
+        /*ExtraArgs=*/SR, "cf_consumed", /*pointers*/1);
+    return;
+  }
 
-  if (IsNSConsumed)
-    D->addAttr(::new (Context)
-                   NSConsumedAttr(AttrRange, Context, SpellingIndex));
-  else
-    D->addAttr(::new (Context)
-                   CFConsumedAttr(AttrRange, Context, SpellingIndex));
+static Sema::RetainOwnershipKind
+parsedAttrToRetainOwnershipKind(const ParsedAttr &AL) {
+  switch (AL.getKind()) {
+  case ParsedAttr::AT_CFConsumed:
+    return Sema::RetainOwnershipKind::CF;
+  case ParsedAttr::AT_OSConsumed:
+    return Sema::RetainOwnershipKind::OS;
+  case ParsedAttr::AT_NSConsumed:
+    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;
 }
 
-static void handleNSReturnsRetainedAttr(Sema &S, Decl *D,
+static void handleXReturnsXRetainedAttr(Sema &S, Decl *D,
                                         const ParsedAttr &AL) {
   QualType ReturnType;
 
-  if (const auto *MD = dyn_cast<ObjCMethodDecl>(D))
+  if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     ReturnType = MD->getReturnType();
-  else if (S.getLangOpts().ObjCAutoRefCount && hasDeclarator(D) &&
-           (AL.getKind() == ParsedAttr::AT_NSReturnsRetained))
+  } 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))
+  } else if (const auto *PD = dyn_cast<ObjCPropertyDecl>(D)) {
     ReturnType = PD->getType();
-  else if (const auto *FD = dyn_cast<FunctionDecl>(D))
+  } else if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     ReturnType = FD->getReturnType();
-  else if (const auto *Param = dyn_cast<ParmVarDecl>(D)) {
+  } 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*/ 2 << AL.getRange();
+          << AL << /*pointer-to-CF-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_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;
   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(S, ReturnType);
+    TypeOK = isValidSubjectOfNSAttribute(ReturnType);
     Cf = false;
     break;
 
   case ParsedAttr::AT_CFReturnsRetained:
   case ParsedAttr::AT_CFReturnsNotRetained:
-    TypeOK = isValidSubjectOfCFAttribute(S, ReturnType);
+    TypeOK = isValidSubjectOfCFAttribute(ReturnType);
+    Cf = true;
+    break;
+
+  case ParsedAttr::AT_OSReturnsRetained:
+  case ParsedAttr::AT_OSReturnsNotRetained:
+    TypeOK = isValidSubjectOfOSAttribute(ReturnType);
     Cf = true;
     break;
   }
 
   if (!TypeOK) {
     if (AL.isUsedAsTypeAttr())
       return;
 
if (!TypeOK) {
     }
     return;
   }
 
   switch (AL.getKind()) {
     default:
       llvm_unreachable("invalid ownership attribute");
     case ParsedAttr::AT_NSReturnsAutoreleased:
-      D->addAttr(::new (S.Context) NSReturnsAutoreleasedAttr(
-          AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+      handleSimpleAttribute<NSReturnsAutoreleasedAttr>(S, D, AL);
       return;
     case ParsedAttr::AT_CFReturnsNotRetained:
-      D->addAttr(::new (S.Context) CFReturnsNotRetainedAttr(
-          AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+      handleSimpleAttribute<CFReturnsNotRetainedAttr>(S, D, AL);
       return;
     case ParsedAttr::AT_NSReturnsNotRetained:
-      D->addAttr(::new (S.Context) NSReturnsNotRetainedAttr(
-          AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+      handleSimpleAttribute<NSReturnsNotRetainedAttr>(S, D, AL);
       return;
     case ParsedAttr::AT_CFReturnsRetained:
-      D->addAttr(::new (S.Context) CFReturnsRetainedAttr(
-          AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+      handleSimpleAttribute<CFReturnsRetainedAttr>(S, D, AL);
       return;
     case ParsedAttr::AT_NSReturnsRetained:
-      D->addAttr(::new (S.Context) NSReturnsRetainedAttr(
-          AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+      handleSimpleAttribute<NSReturnsRetainedAttr>(S, D, AL);
+      return;
+    case ParsedAttr::AT_OSReturnsRetained:
+      handleSimpleAttribute<OSReturnsRetainedAttr>(S, D, AL);
+      return;
+    case ParsedAttr::AT_OSReturnsNotRetained:
+      handleSimpleAttribute<OSReturnsNotRetainedAttr>(S, D, AL);
       return;
   };
 }
 
 static void handleObjCReturnsInnerPointerAttr(Sema &S, Decl *D,
                                               const ParsedAttr &Attrs) {
   const int EP_ObjCMethod = 1;
   const int EP_ObjCProperty = 2;
handleSimpleAttributeWithExclusions<CFAuditedTransferAttr,
                                         CFUnknownTransferAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_CFUnknownTransfer:
     handleSimpleAttributeWithExclusions<CFUnknownTransferAttr,
                                         CFAuditedTransferAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_CFConsumed:
   case ParsedAttr::AT_NSConsumed:
-    handleNSConsumedAttr(S, D, AL);
+  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_NSReturnsAutoreleased:
   case ParsedAttr::AT_NSReturnsNotRetained:
-  case ParsedAttr::AT_CFReturnsNotRetained:
   case ParsedAttr::AT_NSReturnsRetained:
+  case ParsedAttr::AT_CFReturnsNotRetained:
   case ParsedAttr::AT_CFReturnsRetained:
-    handleNSReturnsRetainedAttr(S, D, AL);
+  case ParsedAttr::AT_OSReturnsNotRetained:
+  case ParsedAttr::AT_OSReturnsRetained:
+    handleXReturnsXRetainedAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WorkGroupSizeHint:
     handleWorkGroupSize<WorkGroupSizeHintAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_ReqdWorkGroupSize:
     handleWorkGroupSize<ReqdWorkGroupSizeAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_OpenCLIntelReqdSubGroupSize:

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

for (const auto *TmplAttr : Tmpl->attrs()) {
       Attr *NewAttr = sema::instantiateTemplateAttributeForDecl(
           TmplAttr, Context, *this, TemplateArgs);
       if (NewAttr)
         New->addAttr(NewAttr);
     }
   }
 }
 
+static Sema::RetainOwnershipKind
+attrToRetainOwnershipKind(const Attr *A) {
+  switch (A->getKind()) {
+  case clang::attr::CFConsumed:
+    return Sema::RetainOwnershipKind::CF;
+  case clang::attr::OSConsumed:
+    return Sema::RetainOwnershipKind::OS;
+  case clang::attr::NSConsumed:
+    return Sema::RetainOwnershipKind::NS;
+  default:
+    llvm_unreachable("Wrong argument supplied");
+  }
+}
+
 void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
                             const Decl *Tmpl, Decl *New,
                             LateInstantiatedAttrVec *LateAttrs,
                             LocalInstantiationScope *OuterMostScope) {
   for (const auto *TmplAttr : Tmpl->attrs()) {
     // FIXME: This should be generalized to more than just the AlignedAttr.
     const AlignedAttr *Aligned = dyn_cast<AlignedAttr>(TmplAttr);
     if (Aligned && Aligned->isAlignmentDependent()) {
for (const auto *TmplAttr : Tmpl->attrs()) {
     }
 
     if (auto ABIAttr = dyn_cast<ParameterABIAttr>(TmplAttr)) {
       AddParameterABIAttr(ABIAttr->getRange(), New, ABIAttr->getABI(),
                           ABIAttr->getSpellingListIndex());
       continue;
     }
 
-    if (isa<NSConsumedAttr>(TmplAttr) || isa<CFConsumedAttr>(TmplAttr)) {
-      AddNSConsumedAttr(TmplAttr->getRange(), New,
-                        TmplAttr->getSpellingListIndex(),
-                        isa<NSConsumedAttr>(TmplAttr),
-                        /*template instantiation*/ true);
+    if (isa<NSConsumedAttr>(TmplAttr) || isa<OSConsumedAttr>(TmplAttr) ||
+        isa<CFConsumedAttr>(TmplAttr)) {
+      AddXConsumedAttr(New, TmplAttr->getRange(),
+                       attrToRetainOwnershipKind(TmplAttr),
+                       /*template instantiation=*/true);
       continue;
     }
 
     assert(!TmplAttr->isPackExpansion());
     if (TmplAttr->isLateParsed() && LateAttrs) {
       // Late parsed attributes must be instantiated and attached after the
       // enclosing class has been instantiated.  See Sema::InstantiateClass.
       LocalInstantiationScope *Saved = nullptr;

cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test

 // RUN: clang-tblgen -gen-clang-test-pragma-attribute-supported-attributes -I%src_include_dir %src_include_dir/clang/Basic/Attr.td -o - | FileCheck %s
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 130 attributes:
+// CHECK: #pragma clang attribute supports 132 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUWavesPerEU (SubjectMatchRule_function)
 // CHECK-NEXT: AVRSignal (SubjectMatchRule_function)
 // CHECK-NEXT: AbiTag (SubjectMatchRule_record_not_is_union, SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_namespace)
 // CHECK-NEXT: Alias (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: AlignValue (SubjectMatchRule_variable, SubjectMatchRule_type_alias)

 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
 // 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)
+// CHECK-NEXT: OSReturnsRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // 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)

cfe/trunk/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_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_not_retained)) S *ret_not_retained() {
+  return nullptr;
+}
+
+__attribute__((os_returns_not_retained)) S ret_not_retained_value() { // expected-warning{{'os_returns_not_retained' attribute only applies to functions that return a pointer}}
+  return {};
+}
+
+void accept_consumed_arg(__attribute__((os_consumed)) S *arg) {}
+
+void accept_consumed_arg_by_value(__attribute__((os_consumed)) S arg) {} // expected-warning{{os_consumed attribute only applies to pointer parameters}}
+
+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}}
+
+__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}}

cfe/trunk/test/Sema/attr-osobject.mm

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+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