[attributes] [analyzer] Add handle related attributes

[xazax.hun]

Summary

These are three new attributes that I plan to use in a Fuchsia specific check. I did not give them Fuchsia specific names as they might be useful for other APIs such as Posix as well. In case that is a concern I can rename them to be more specific to the check.

[NoQ]

clang/lib/Sema/SemaDeclAttr.cpp

7408

The next obvious step here would be to type-check the declaration to make sure that it's actually a handle (and emit a warning if it isn't).

[xazax.hun]

clang/lib/Sema/SemaDeclAttr.cpp

7408

Yeah, I do agree, but I think this depends on the role of the attribute. Adding a type check would make this more restrictive in a sense other users who want to write for example a Posix API checker and want to reuse this attribute might not be able to do so without touching the type-check code. Which may or may not be good. I do not have any strong feelings about either of the directions.

[NoQ]

clang/lib/Sema/SemaDeclAttr.cpp

7408

So you envision this working on completely arbitrary types? Fair!

Maybe check that when AcquireHandleAttr is on a parameter, it's actually an out-parameter?

[xazax.hun]

clang/lib/Sema/SemaDeclAttr.cpp

7408

Hmm, I think this makes a lot of sense! So basically, I would exclude primitive, non-pointer types that are taken by value.

[aaron.ballman]

This is also missing tests for all the Sema handling (that the attributes only appertain to the specified subjects, accept no arguments, etc).

clang/include/clang/Basic/Attr.td

3477

What about function-like interfaces such as lambdas, blocks, or other callable objects that are not a FunctionDecl?

3482

Should this have a subject limiting it to parameters?

3487

Should this have a subject limiting it to parameters?

clang/include/clang/Basic/AttrDocs.td

4703

What is a "handle"? I think some introduction docs are needed.

[xazax.hun]

• Check trivial cases when the acquire_handle attribute is not on an output parameter.

[xazax.hun]

clang/include/clang/Basic/Attr.td

3477

Good point!

3482

My understanding was that the subject list is "inherited" from InheritableParamAttr, which already has it limited to parameters. Is this not the case?

clang/include/clang/Basic/AttrDocs.td

4703

Good point. Do you prefer to copy and paste the introduction to all attributes or is it enough to only have it for one of them?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3482

It doesn't seem to be the case looking at Attr.td:557 or thereabouts. Adding tests for the attributes should clarify whether the change is needed or not, too.

clang/include/clang/Basic/AttrDocs.td

4703

You can set up a documentation category to put all the related attributes under, and that category can have the introductory documentation. You can see an example of this with the calling convention attributes. There is a DocCatCallingConvs that is then the Category for all the other attributes.

[xazax.hun]

• Address review comments.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3477

This get you partway there -- FunctionLike allows functions and function pointers, but I don't think it allows lambdas or blocks. You should add some test cases for all of these (including function pointers) to verify you're able to mark the entities you want and get the behavior you're after.

For instance, on function pointers and lambdas, this may impact the function *type* which is a good design question to be thinking about. What should happen with code like:

[[clang::acquire_handle]] void int open(const char *);
int (*fp)(const char *) = open;

If the attribute is a type attribute as opposed to a declaration attribute, then you can think about diagnostics in these sort of cases. However, if it is a type attribute, there is more work involved in hooking it up to the type system.

FWIW, this attribute smells like it should be a type attribute rather than a declaration attribute because I can imagine wanting to use this with function pointers and not losing the analysis capabilities (esp for things like callback functions).

3483

One test that you should add is whether this case is properly handled:

void (*fp)(int whatever [[clang::uses_handle]]);
[](int whatever [[clang::uses_handle]]){};

e.g., when it's on a parameter declaration of function pointer type or a lambda, does everything still behave as expected?

clang/include/clang/Basic/AttrDocs.td

4702

sockets, processes -> sockets, and processes

clang/lib/Sema/SemaDeclAttr.cpp

6606

const auto *

6607

I'm skeptical of this. I think a better check is if the type is a pointer or reference -- was there a reason you didn't go with that approach?

[xazax.hun]

clang/lib/Sema/SemaDeclAttr.cpp

6607

The reason why I do not want to restrict this to pointers and references is that the users might have a custom wrapper type to deal with handles that might be passed by value. Or the user might pass an iterator by value which points to a handle. These are not extremely likely, but I did not want to diagnose those cases. What do you think?

[xazax.hun]

• Address review comments.

[xazax.hun]

clang/include/clang/Basic/Attr.td

3477

I see your point. I have, however, some concerns making this part of the type system. I think it would be great to let the users add the annotations gradually without any diagnostic. For example:

void my_function_with_callback( int (*callback)(int __attribute__((handle_use)) );

...

my_function_with_callback(already_annotated);
my_function_with_callback(not_yet_annotated); // Do we intend to give a diagnostics here?

What do you think?

3477

After some digging it looks like I cannot apply any attributes to lambdas. For example, you cannot even add [[noreturn]] to a lambda, because the language only supports type attributes in that position.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3477

After some digging it looks like I cannot apply any attributes to lambdas.

That's what got me thinking about making this a type attribute instead. It would be a shame for you to not be able to mark lambdas.

I have, however, some concerns making this part of the type system.

Understandable -- type attributes are not much fun. ;-) I think your concern is justified, but you have the control to allow or disallow whatever you'd like in terms of the semantics of the type attribute. Personally, I would want that use case to give a diagnostic because that use may close the handle, for instance. Or it could be creating/releasing a handle instead of just using one, etc.

For instance, one idea (and it may not be a good one) is that you could diagnose any implicit conversion between the function pointer types, but still allow explicit conversion between them. This gives users a transition path -- they can explicitly add the type cast in the places they've not yet got the annotations added yet. They could even use a macro to perform the cast, giving them a handy marker of what code still needs to be fixed.

clang/lib/Sema/SemaDeclAttr.cpp

6607

I guess I was envisioning the value type being the issue, but I suppose you could have some sort of reference-counted object where the copy is not problematic. But this is still a bit of a strange constraint -- it disallows putting it on an integer parameter type, but not, say a float or a const char *?

Have you considered requiring the handle type to be annotated itself, so that you can verify the parameter is actually a handle as opposed to a mistake?

[xazax.hun]

• Convert to type attributes.

[xazax.hun]

@aaron.ballman

Type attributes are definitely more flexible as in they can be applied in more contexts. These attributes, however, make no sense outside of a function declaration, or maybe a type alias. So I feel like we could argue on both sides. I made a minimal version of the attribute so it can potentially unblock dependent revisions (the static analyzer changes) and plan to add more diagnostics in follow-up patches. What do you think?

[xazax.hun]

Ping.

[xazax.hun]

In the meantime, I realized lambdas are actually not that important. Lambdas are only usable in C++, and in C++ one should prefer to use move only RAII wrapper for handles. This reduces the problem into a use after move and we already have a check for that. These annotations are more important for C, where we do not have language features to mitigate these problems. The function pointer argument is still valid though.

[aaron.ballman]

In D70469#1766084, @xazax.hun wrote:

@aaron.ballman

Type attributes are definitely more flexible as in they can be applied in more contexts. These attributes, however, make no sense outside of a function declaration, or maybe a type alias.

Function pointers are a primary case for what I was thinking of, and those are neither a function declaration nor a type alias.

So I feel like we could argue on both sides. I made a minimal version of the attribute so it can potentially unblock dependent revisions (the static analyzer changes) and plan to add more diagnostics in follow-up patches. What do you think?

Thank you! I think this is a more flexible way forward.

In D70469#1770041, @xazax.hun wrote:

In the meantime, I realized lambdas are actually not that important. Lambdas are only usable in C++, and in C++ one should prefer to use move only RAII wrapper for handles.

Generally yes, but thinking of things like file descriptors (which are also handles), it seems like overkill to expect someone to wrap those in a separate type.

This reduces the problem into a use after move and we already have a check for that. These annotations are more important for C, where we do not have language features to mitigate these problems. The function pointer argument is still valid though.

Okay, that's a fair point.

clang/include/clang/Basic/Attr.td

3475

These probably should be DeclOrTypeAttr because they can be applied to a parameter (which is a declaration) or a type.

clang/include/clang/Basic/AttrDocs.td

4700

The docs should clarify when they say "function" that they mean the function type rather than the function declaration.

clang/lib/Sema/SemaType.cpp

7436

You should be able to pass in Attr directly instead of Attr.getAttrName()->getName(), I believe. Also, I'd prefer the non-function be put into the diagnostic text itself with a %select if we need to vary it.

clang/test/Sema/attr-handles.cpp

8

Are you planning to implement the TODOs as part of this patch?

[xazax.hun]

• Make the annotation acceptable on both types and declarations.
• Fix some TODOs.
• Teach TypePrinter to print the annotations.
• Address review comments.

clang/lib/Sema/SemaType.cpp

7436

Indeed. But only passing Attr would result in duplicated '. I'm reusing the message, so I ended up not changing this part.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

7444

Is this correct, or should it be if (!CurType->isFunctionType())?

clang/test/Sema/attr-handles.cpp

21–22

These diagnostics look incorrect to me -- I thought we wanted the attribute to apply to function types?

You should also add some test cases where the attribute is applied to a function pointer type.

23

Please add the newline to the end of the file.

[xazax.hun]

clang/test/Sema/attr-handles.cpp

21–22

I think this one can be a bit confusing. We can apply this to the types of the parameter or the return type. I thought restricting to users to put it either on the parameter or the return type would make it more obvious what is the target of this attribute. In case this is not idiomatic, I can let the user to attach it to the function type.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

7444

I see now that this is only incorrect for the acquire attribute, but not the other two.

clang/test/Sema/attr-handles.cpp

4

Can you add some test cases showing that these work on a typedef declaration?

21–22

Ah, I see -- acquire can be on a function type, but use and release are on the parameter. You are missing some test cases:

int correct() [[clang::acquire_handle]]; // Should work, applies to the function type
int (*fp [[clang::acquire_handle]])(); // Should work, applies to the function type

[xazax.hun]

clang/lib/Sema/SemaType.cpp

7444

I think we might consider it incorrect for the acquire as well. Because if we let the user put acquire on the function, it is ambiguous where to put the annotation. If we do not let them put on the function they are always forced to put on the return type to express this. But in case this kind of ambiguity is a feature and not a bug I can change the behavior.

[xazax.hun]

• Make sure typedefs are supported.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

7444

I guess I was considering the semantics as being acquire on a function type applies to the non-void return value being the handle and release on a function type applies to the only parameter being passed to the function.

I don't think it makes sense to apply acquire or release semantics to a handle type itself because that's not really part of the type identity itself (it's not an "acquiring handle" or a "releasing handle" -- I would think it's *both* at the same time), so I think that's been throwing me for a loop with the latest round of patches here. What I was imagining was that you should mark any place a handle is created or released, which works almost-fine for parameters (because you can mark the parameter declarations with an attribute). It doesn't quite work for function pointers, however, because those don't have parameters. e.g., void (*fp)([[clang::acquire_handle]] int &) won't do what you expect, I believe. However, for functions which return the acquired handle, there is no good place to write the attribute except on the function itself (because, again, it's not a property of the return type, but of the specific function's return type). For those cases, I imagine you want the function *type* to be what codifies that a handle is returned, because then it will work with function pointers. For *using* a handle, I'm not certain what the benefit is to having an attribute on each use -- that seems likely for a user to forget to add an annotation somewhere and get incorrect behavior. I would normally suggest that the attribute be applied to the type declaration of the handle (e.g., through a typedef), but if you want this to work with handles coming from system headers (like file descriptors or sockets, etc) then will this approach work?

[NoQ]

It still mildly worries me that the attributes aren't truly reusable, as the exact meaning of the attribute depends on the domain. In particular, every domain is expected to have different approaches to error handling, i.e. a function that creates or destroys a handle may fail to, respectively, create or destroy the handle, but instead indicate the failure in a domain-specific manner, eg. through magical return values or null handle or errno or whatever.

@aaron.ballman, do you think this is a problem? Should we rather go for an attribute name that's obviously domain-specific (eg., __attribute__((fuchsia_acquire_handle))), or is it ok to re-use attributes without re-using their exact meaning?

[aaron.ballman]

In D70469#1776523, @NoQ wrote:

It still mildly worries me that the attributes aren't truly reusable, as the exact meaning of the attribute depends on the domain. In particular, every domain is expected to have different approaches to error handling, i.e. a function that creates or destroys a handle may fail to, respectively, create or destroy the handle, but instead indicate the failure in a domain-specific manner, eg. through magical return values or null handle or errno or whatever.

@aaron.ballman, do you think this is a problem? Should we rather go for an attribute name that's obviously domain-specific (eg., __attribute__((fuchsia_acquire_handle))), or is it ok to re-use attributes without re-using their exact meaning?

That may be part of why I keep pushing back on this addition -- it seems like these are general purpose attributes that can be used to identify what a handle is and where a handle is obtained/released. Or these could be specific to a particular coding guideline's definition of a handle and associated semantics. If the goal is to only support a limited set of use cases, then I think something like [[clang::fucschia_acquire_handle]] makes more sense. If the goal is to provided general utilities for the static analyzer to reason about handles, then I think we want the more generalized names.

[xazax.hun]

In D70469#1777858, @aaron.ballman wrote:

In D70469#1776523, @NoQ wrote:

It still mildly worries me that the attributes aren't truly reusable, as the exact meaning of the attribute depends on the domain. In particular, every domain is expected to have different approaches to error handling, i.e. a function that creates or destroys a handle may fail to, respectively, create or destroy the handle, but instead indicate the failure in a domain-specific manner, eg. through magical return values or null handle or errno or whatever.

@aaron.ballman, do you think this is a problem? Should we rather go for an attribute name that's obviously domain-specific (eg., __attribute__((fuchsia_acquire_handle))), or is it ok to re-use attributes without re-using their exact meaning?

That may be part of why I keep pushing back on this addition -- it seems like these are general purpose attributes that can be used to identify what a handle is and where a handle is obtained/released. Or these could be specific to a particular coding guideline's definition of a handle and associated semantics. If the goal is to only support a limited set of use cases, then I think something like [[clang::fucschia_acquire_handle]] makes more sense. If the goal is to provided general utilities for the static analyzer to reason about handles, then I think we want the more generalized names.

Basically, I think the exact semantics belong to the static analysis and the annotation is only there to supplement some information about the code that is not available in the type system. So in the sense I envisioned these attributes as a general way to encode knowledge about an API without relying on any semantics from any analysis. And the FuchsiaHandleChecker would be a first analysis to consume these handles. Inevitable, different analyses have to interpret these slightly differently when it comes to error handling and other corner cases. This is the result of APIs being fundamentally different. The question is, does this justify having a separate set of annotations for all handles, like posix_handle_acquire, fuchsia_handle_acquire? I do not have strong feelings and I am also OK with going a Fuchsia specific name. It is not that hard to add a new spelling without code repetition. Also, having separate spelling could provide some additional benefits, e.g. we could check of someone is trying to close a posix handle using a fuchsia API. I think both directions have some pros and cons the ultimate questions is what do we care about more. Assigning more specific semantics to annotations (as opposed to the analyses only) or reducing the annotation vocabulary.

[NoQ]

What about __attribute__((acquire_handle("fuchsia")))?

[xazax.hun]

In D70469#1778609, @NoQ wrote:

What about __attribute__((acquire_handle("fuchsia")))

I would by fine with this as well. Aaron?

[xazax.hun]

clang/lib/Sema/SemaType.cpp

7444

Hmm. The problem is that a function might do multiple things with multiple handles, for example in Fuchsia there are functions that replacing a handle with another one. So I am afraid naively putting the attributes on the function type might end up being ambiguous sometimes. I totally agree with your argument about the annotation not being part of the type identity. If we do want to put this on the function type maybe we could do something similar to nonnull and enumerate parameter indices? What do you think?

The use annotation might be a bit misleading. The user does not necessarily need to annotate all uses.
Let's consider the following function and a call:

void f(int *handle);
close_handle(handle);
f(&handle); // What will this do?

The function f would be free to do anything, like store the address of the handle in a global data structure, overwrite the closed handle and so on. The purpose of the use annotation is to tell the analyzer that none of this will happen, so we can safely diagnose a use after release. So the original idea was to annotate those functions that are guaranteed to never close, overwrite etc the handle.
For unannotated functions the analyzer will be conservative and assume that the code is OK.

[NoQ]

clang/lib/Sema/SemaType.cpp

7444

I heard that type attributes are relatively scary and require a lot of work to implement, and nonnull/_Nonnull is definitely one of the historical examples of why they're scary. Like, you don't want to mess with the type system and find yourself explaining whether it's still the same type or a different type simply because it wears an attribute. Declaration attributes are much more straightforward in this sense.

[aaron.ballman]

In D70469#1779906, @xazax.hun wrote:

In D70469#1778609, @NoQ wrote:

What about __attribute__((acquire_handle("fuchsia")))

I would by fine with this as well. Aaron?

I would also be fine with that. I don't have a strong opinion on whether to use a string literal or an identifier as the argument, but I slightly swing towards string literal so you could do something like "C++ Core Guideline" where there are spaces or punctuators.

clang/lib/Sema/SemaType.cpp

7444

I heard that type attributes are relatively scary and require a lot of work to implement

They're harder to implement than declaration attributes because there's more to think about, but that's not a reason to not use a type attribute when that's the correct semantics we want.

7444

The purpose of the use annotation is to tell the analyzer that none of this will happen, so we can safely diagnose a use after release. So the original idea was to annotate those functions that are guaranteed to never close, overwrite etc the handle.

Thank you for the explanation, that helps me understand better.

I think the default for "use" may be backward -- it seems to me that less functions will escape/close a handle than will operate on a handle. Do you have any data to back up my gut feeling? :-D I sort of wonder if we want the model to be: 1) mark a type as being a handle of a specific type (POSIX, fuschia, Win32, etc), 2a) mark a parameter declaration as creating a handle (the parameter type specifies the handle type), 2b) mark a function type as its return value creating a handle (the return type specifies the handle type), 3a) mark a parameter declaration as releasing a handle (the parameter type specifies the handle type), 3b) mark a function type as releasing a handle (all parameters of handle types are considered to be released), 4) mark a parameter declaration as possibly mutating the state of the handle (closing it, changing its identity, escaping it, etc).

Usage would be something like:

typedef int PosixHandle [[clang::handle_type("POSIX")]];

PosixHandle open(const char *) [[clang::acquire_handle]];
bool open2(const char *, [[clang::acquire_handle]] PosixHandle&);

void close(PosixHandle) [[clang::release_handle]];
void close2([[clang::release_handle]] PosixHandle);

void typical_use(PosixHandle);
void atypical_use([[clang::modifies_handle]] PosixHandle&);

// Maybe we want to allow these too?
int open3(const char *) [[clang::acquire_handle("POSIX")]];
void close3([[clang::release_handle("POSIX") int);
void atypical_use2([[clang::modifies_handle("POSIX") int&);

However, when I think about this, this smells an awful lot like some existing attributes we already support: capabilities. We currently use them for thread safety analysis, but there's no reason they shouldn't work out of the box for what you want (https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#basic-concepts-capabilities). The basic idea behind them is that you specify a capability like "handle" and then specify what operations grant that capability (acquire the handle) or revoke it (release the handle). It may be worth exploring whether we can reuse these annotations for what you're trying to accomplish.

[xazax.hun]

clang/lib/Sema/SemaType.cpp

7444

While it is true that less function will escape/close handles, this is also a trade-off. If we assume all unknown functions will close/escape, the check will be less noisy with unannotated/patially annotated codebase. If we assume that unknown function will not escape/close, we will get more false positives unless the whole code base is annotated. I chose this to support gradual introduction of annotations.

Ok. So how the handle works on declarations it is pretty clear. If we put it on a parameter, it affects that parameter, if we put it on a function, it affects the return value. The question is, how to deal with types.

2b) mark a function type as its return value creating a handle (the return type specifies the handle type
3b) mark a function type as releasing a handle (all parameters of handle types are considered to be released)

This does not really work. In case of Fuchsia, none of the syscalls actually return handles. Handles are acquired using output parameters and there are syscalls that acquire multiple handles. There are also syscalls that replaces handles, i.e. whatever we do at the type level we really need the paramter granularity. Any simplification like if we annotate the function type as such and such will affect all the parameters will not work.

However, when I think about this, this smells an awful lot like some existing attributes we already support: capabilities.

I did look into capabilities, and while I am not an expert in this area I found the following problems:

• As far as I understand capabilities also only work well with fully annotated code, and gradually adding annotations can result in lots of false positives.
• As far as I understand, they do not have paramter level granularity, which is crucial for Fuchsia's APIs.

I did not want to generalize capability attributes as I would probably need to update all the related analysis as well.

[xazax.hun]

• Added string argument to specify the handle type.
• Only AcquireHandleAttr can be a type attribute and it can only be applied to function types.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

3475

I have no idea whether this is a problem or not (and I'm not certain if you know either) -- how bad is it that this is DeclOrTypeAttr in the case where it's an inherited parameter with the attribute? The other attributes use InheritableParamAttr, but the only thing they apply to is a parameter so that's easy. I don't know what should be done in this case, or if it's an issue.

I think a test case for this scenario would be:

void func([[clang::acquires_handle("foo") int *a);

void func(int *a) {
  // a should be marked as acquires_handle in the AST should you do an AST dump
}

clang/lib/Sema/SemaType.cpp

7638

Can you be sure to add a test where the attribute is written in the type position for a non-function type to ensure you get a reasonable diagnostic?

e.g., int [[clang::acquire_handle("")]] a;

[aaron.ballman]

LGTM with the nits fixed, thank you!

clang/include/clang/Basic/Attr.td

3475

I verified that this is an issue, but it's not a new one. [[clang::lifetimebound]] has the same issue. You should add a FIXME comment here and a test case with a FIXME comment so we don't lose track of this, but it doesn't seem critical to fix right now.

[xazax.hun]

Thanks for the review! :)

[paulherman]

clang/include/clang/Basic/AttrDocs.td

4724

Sorry for the drive-by comment, but I believe that the code snippets are out of date since the addition of the string param. Would you mind clarifying what the param should do -- is it a project name such as "fuchsia" or is it a name of the handle such as "my_awesome_pipe"? I'd be happy to take the fix, but not sure which way to go.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4724

I understand it to be the name of the handle, not a project-specific name.

clang/include/clang/Basic/Attr.td

 }
 
 def NoBuiltin : Attr {
   let Spellings = [Clang<"no_builtin">];
   let Args = [VariadicStringArgument<"BuiltinNames">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [NoBuiltinDocs];
 }
+
+// FIXME: This attribute is not inheritable, it will not be propagated to
+// redecls. [[clang::lifetimebound]] has the same problems. This should be
+// fixed in TableGen (by probably adding a new inheritable flag).
+def AcquireHandle : DeclOrTypeAttr {
+  let Spellings = [Clang<"acquire_handle">];
+  let Args = [StringArgument<"HandleType">];
+  let Subjects = SubjectList<[Function, TypedefName, ParmVar]>;
+  let Documentation = [AcquireHandleDocs];
+}
+
+def UseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"use_handle">];
+  let Args = [StringArgument<"HandleType">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [UseHandleDocs];
+}
+
+def ReleaseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"release_handle">];
+  let Args = [StringArgument<"HandleType">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [ReleaseHandleDocs];
+}

clang/include/clang/Basic/AttrDocs.td

void bar(char* data, size_t count) __attribute__((no_builtin("memcpy"))) {
     // The compiler is allowed to convert the loop into
     // `__builtin_memset(data, 0xFE, count);` but cannot generate any
     // `__builtin_memcpy`
     for (size_t i = 0; i < count; ++i)
       data[i] = 0xFE;
   }
   }];
 }
+
+def HandleDocs : DocumentationCategory<"Handle Attributes"> {
+  let Content = [{
+Handles are a way to identify resources like files, sockets, and processes.
+They are more opaque than pointers and widely used in system programming. They
+have similar risks such as never releasing a resource associated with a handle,
+attempting to use a handle that was already released, or trying to release a
+handle twice. Using the annotations below it is possible to make the ownership
+of the handles clear: whose responsibility is to release them. They can also
+aid static analysis tools to find bugs.
+  }];
+}
+
+def AcquireHandleDocs : Documentation {
+  let Category = HandleDocs;
+  let Content = [{
+If this annotation is on a function or a function type it is assumed to return
+a new handle. In case this annotation is on an output parameter,
+the function is assumed to fill the corresponding argument with a new
+handle.
+
+.. code-block:: c++
+
+  // Output arguments from Zircon.
+  zx_status_t zx_socket_create(uint32_t options,
+                               zx_handle_t __attribute__((acquire_handle)) * out0,
+                               zx_handle_t* out1 [[clang::acquire_handle]]);
+
+
+  // Returned handle.
+  [[clang::acquire_handle]] int open(const char *path, int oflag, ... );
+  int open(const char *path, int oflag, ... ) __attribute__((acquire_handle));
+  }];
+}
+
+def UseHandleDocs : Documentation {
+  let Category = HandleDocs;
+  let Content = [{
+A function taking a handle by value might close the handle. If a function
+parameter is annotated with `use_handle` it is assumed to not to change
+the state of the handle. It is also assumed to require an open handle to work with.
+
+.. code-block:: c++
+
+  zx_status_t zx_port_wait(zx_handle_t handle [[clang::use_handle]],
+                           zx_time_t deadline,
+                           zx_port_packet_t* packet);
+  }];
+}
+
+def ReleaseHandleDocs : Documentation {
+  let Category = HandleDocs;
+  let Content = [{
+If a function parameter is annotated with `release_handle` it is assumed to
+close the handle. It is also assumed to require an open handle to work with.
+
+.. code-block:: c++
+
+  zx_status_t zx_handle_close(zx_handle_t handle [[clang::release_handle]]);
+  }];
+}

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_invalid_attribute_on_virtual_function : Error<
   "%0 attribute cannot be applied to virtual functions">;
 def warn_declspec_allocator_nonpointer : Warning<
   "ignoring __declspec(allocator) because the function return type %0 is not "
   "a pointer or reference type">, InGroup<IgnoredAttributes>;
 def err_cconv_incomplete_param_type : Error<
   "parameter %0 must have a complete type to use function %1 with the %2 "
   "calling convention">;
+def err_attribute_output_parameter : Error<
+  "attribute only applies to output parameters">;
 
 def ext_cannot_use_trivial_abi : ExtWarn<
   "'trivial_abi' cannot be applied to %0">, InGroup<IgnoredAttributes>;
 
 // Availability attribute
 def warn_availability_unknown_platform : Warning<
   "unknown platform %0 in availability macro">, InGroup<Availability>;
 def warn_availability_version_ordering : Warning<

clang/lib/AST/TypePrinter.cpp

case attr::PreserveMost:
     break;
 
   case attr::PreserveAll:
     OS << "preserve_all";
     break;
   case attr::NoDeref:
     OS << "noderef";
     break;
+  case attr::AcquireHandle:
+    OS << "acquire_handle";
+    break;
   }
   OS << "))";
 }
 
 void TypePrinter::printObjCInterfaceBefore(const ObjCInterfaceType *T,
                                            raw_ostream &OS) {
   OS << T->getDecl()->getName();
   spaceBeforePlaceHolder(OS);

clang/lib/Sema/SemaDeclAttr.cpp

if (!RetTy->isPointerType() && !RetTy->isReferenceType()) {
           << AL.getRange() << RetTy;
       return;
     }
   }
 
   handleSimpleAttribute<MSAllocatorAttr>(S, D, AL);
 }
 
+static void handeAcquireHandleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (AL.isUsedAsTypeAttr())
+    return;
+  // Warn if the parameter is definitely not an output parameter.
+  if (const auto *PVD = dyn_cast<ParmVarDecl>(D)) {
+    if (PVD->getType()->isIntegerType()) {
+      S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
+          << AL.getRange();
+      return;
+    }
+  }
+  StringRef Argument;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Argument))
+    return;
+  D->addAttr(AcquireHandleAttr::Create(S.Context, Argument, AL));
+}
+
+template<typename Attr>
+static void handleHandleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef Argument;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Argument))
+    return;
+  D->addAttr(Attr::Create(S.Context, Argument, 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,
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
 
   case ParsedAttr::AT_MSAllocator:
     handleMSAllocatorAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_ArmMveAlias:
     handleArmMveAliasAttr(S, D, AL);
     break;
+
+  case ParsedAttr::AT_AcquireHandle:
+    handeAcquireHandleAttr(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_ReleaseHandle:
+    handleHandleAttr<ReleaseHandleAttr>(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_UseHandle:
+    handleHandleAttr<UseHandleAttr>(S, D, AL);
+    break;
   }
 }
 
 /// ProcessDeclAttributeList - Apply all the decl attributes in the specified
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
                                     const ParsedAttributesView &AttrList,
                                     bool IncludeCXX11Attributes) {

clang/lib/Sema/SemaType.cpp

 }
 
 static bool isAddressSpaceKind(const ParsedAttr &attr) {
   auto attrKind = attr.getKind();
 
   return attrKind == ParsedAttr::AT_AddressSpace ||
          attrKind == ParsedAttr::AT_OpenCLPrivateAddressSpace ||
          attrKind == ParsedAttr::AT_OpenCLGlobalAddressSpace ||
          attrKind == ParsedAttr::AT_OpenCLLocalAddressSpace ||
          attrKind == ParsedAttr::AT_OpenCLConstantAddressSpace ||
          attrKind == ParsedAttr::AT_OpenCLGenericAddressSpace;
 }
 
 static void processTypeAttrs(TypeProcessingState &state, QualType &type,
                              TypeAttrLocation TAL,
                              ParsedAttributesView &attrs) {
   // Scan through and apply attributes to this type where it makes sense.  Some
   // attributes (such as __address_space__, __vector_size__, etc) apply to the
   // type, but others can be present in the type specifiers even though they
   // apply to the decl.  Here we apply type attributes and ignore the rest.
 
   // This loop modifies the list pretty frequently, but we still need to make
   // sure we visit every element once. Copy the attributes list, and iterate
   // over that.
   ParsedAttributesView AttrsCopy{attrs};
FUNCTION_TYPE_ATTRS_CASELIST:
       // declaration-specifiers.
       if (TAL == TAL_DeclSpec)
         distributeFunctionTypeAttrFromDeclSpec(state, attr, type);
 
       // Otherwise, handle the possible delays.
       else if (!handleFunctionTypeAttr(state, attr, type))
         distributeFunctionTypeAttr(state, attr, type);
       break;
+    case ParsedAttr::AT_AcquireHandle: {
+      if (!type->isFunctionType())
+        return;
+      StringRef HandleType;
+      if (!state.getSema().checkStringLiteralArgumentAttr(attr, 0, HandleType))
+        return;
+      type = state.getAttributedType(
+          AcquireHandleAttr::Create(state.getSema().Context, HandleType, attr),
+          type, type);
+      attr.setUsedAsTypeAttr();
+      break;
+    }
     }
 
     // Handle attributes that are defined in a macro. We do not want this to be
     // applied to ObjC builtin attributes.
     if (isa<AttributedType>(type) && attr.hasMacroIdentifier() &&
         !type.getQualifiers().hasObjCLifetime() &&
         !type.getQualifiers().hasObjCGCAttr() &&
         attr.getKind() != ParsedAttr::AT_ObjCGC &&

clang/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 the following 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: AcquireHandle (SubjectMatchRule_function, SubjectMatchRule_type_alias, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: Alias (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: AlignValue (SubjectMatchRule_variable, SubjectMatchRule_type_alias)
 // CHECK-NEXT: AllocSize (SubjectMatchRule_function)
 // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: AlwaysInline (SubjectMatchRule_function)
 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)

 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
+// CHECK-NEXT: ReleaseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: SpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftContext (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftErrorResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftIndirectResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
+// CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportModule (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportName (SubjectMatchRule_function)
 // CHECK-NEXT: WorkGroupSizeHint (SubjectMatchRule_function)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: End of supported attributes.

clang/test/Sema/attr-handles.cpp

+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+// Decl annotations.
+void f(int *a __attribute__((acquire_handle("Fuchsia"))));
+void (*fp)(int handle [[clang::use_handle("Fuchsia")]]);
+auto lambda = [](int handle [[clang::use_handle("Fuchsia")]]){};
+void g(int a __attribute__((acquire_handle("Fuchsia")))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle))); // expected-error {{'acquire_handle' attribute takes one argument}}
+void h(int *a __attribute__((acquire_handle(1)))); // expected-error {{attribute requires a string}}
+void h(int *a __attribute__((acquire_handle("RandomString", "AndAnother")))); // expected-error {{'acquire_handle' attribute takes one argument}}
+__attribute__((release_handle("Fuchsia"))) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle("Fuchsia"))) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle("Fuchsia"))); // expected-warning {{'acquire_handle' attribute only applies to functions, typedefs, and parameters}}
+int (* __attribute__((acquire_handle("Fuchsia"))) fpt)(char *); // expected-warning {{'acquire_handle' attribute only applies to functions, typedefs, and parameters}}
+
+// Type annotations.
+auto lambdat = [](int handle __attribute__((use_handle("Fuchsia"))))
+    __attribute__((acquire_handle("Fuchsia"))) -> int { return 0; };
+int __attribute((acquire_handle("Fuchsia"))) ta; // expected-warning {{'acquire_handle' attribute only applies to functions, typedefs, and parameters}}
+
+// Typedefs.
+typedef int callback(char *) __attribute__((acquire_handle("Fuchsia")));