[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:4724
+  zx_status_t zx_socket_create(uint32_t options,
+   zx_handle_t __attribute__((acquire_handle)) * 
out0,
+   zx_handle_t* out1 [[clang::acquire_handle]]);

paulherman wrote:
> 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.
I understand it to be the name of the handle, not a project-specific name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Paul Herman via Phabricator via cfe-commits]

paulherman added inline comments.
Herald added subscribers: manas, steakhal, jdoerfert, ASDenysPetrov, phosek.



Comment at: clang/include/clang/Basic/AttrDocs.td:4724
+  zx_status_t zx_socket_create(uint32_t options,
+   zx_handle_t __attribute__((acquire_handle)) * 
out0,
+   zx_handle_t* out1 [[clang::acquire_handle]]);

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added a comment.

Thanks for the review! :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe17b30a7957: [attributes][analyzer] Add annotations for 
handles. (authored by xazax.hun).

Changed prior to commit:
  https://reviews.llvm.org/D70469?vs=234341=234940#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,22 @@
+// 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")));
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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)
@@ -128,6 +129,7 @@
 // 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)
@@ -145,6 +147,7 @@
 // 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)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7634,6 +7634,18 @@
   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 = 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with the nits fixed, thank you!




Comment at: clang/include/clang/Basic/Attr.td:3475
+
+def AcquireHandle : DeclOrTypeAttr {
+  let Spellings = [Clang<"acquire_handle">];

aaron.ballman wrote:
> 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
> }
> ```
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3475
+
+def AcquireHandle : DeclOrTypeAttr {
+  let Spellings = [Clang<"acquire_handle">];

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
}
```



Comment at: clang/lib/Sema/SemaType.cpp:7618
+case ParsedAttr::AT_AcquireHandle: {
+  if (!type->isFunctionType())
+return;

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;`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 234341.
xazax.hun added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,21 @@
+// 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; };
+
+// Typedefs.
+typedef int callback(char *) __attribute__((acquire_handle("Fuchsia")));
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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)
@@ -128,6 +129,7 @@
 // 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)
@@ -145,6 +147,7 @@
 // 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)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7614,6 +7614,18 @@
   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 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

aaron.ballman wrote:
> aaron.ballman wrote:
> > NoQ wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > xazax.hun wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Is this correct, or should it be `if 
> > > > > > > > (!CurType->isFunctionType())`?
> > > > > > > I see now that this is only incorrect for the acquire attribute, 
> > > > > > > but not the other two.
> > > > > > 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.
> > > > > 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?
> > > > 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(); // 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. 
> > > 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 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

NoQ wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > Is this correct, or should it be `if (!CurType->isFunctionType())`?
> > > > > I see now that this is only incorrect for the acquire attribute, but 
> > > > > not the other two.
> > > > 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.
> > > 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?
> > 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(); // 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. 
> I heard that type attributes are relatively scary and require a lot of work 
> to implement, and 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > Is this correct, or should it be `if (!CurType->isFunctionType())`?
> > > > I see now that this is only incorrect for the acquire attribute, but 
> > > > not the other two.
> > > 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.
> > 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?
> 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(); // 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. 
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > Is this correct, or should it be `if (!CurType->isFunctionType())`?
> > > I see now that this is only incorrect for the acquire attribute, but not 
> > > the other two.
> > 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.
> 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?
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(); // 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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D70469#1778609 , @NoQ wrote:

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


I would by fine with this as well. Aaron?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

xazax.hun wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Is this correct, or should it be `if (!CurType->isFunctionType())`?
> > I see now that this is only incorrect for the acquire attribute, but not 
> > the other two.
> 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.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232873.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

- Make sure typedefs are supported.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+// Decl annotations.
+void f(int *a __attribute__((acquire_handle)));
+void (*fp)(int handle [[clang::use_handle]]);
+auto lambda = [](int handle [[clang::use_handle]]){};
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle(1; // expected-error {{'acquire_handle' attribute takes no arguments}}
+__attribute__((release_handle)) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle)) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle)); // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, parameters, and typedefs}}
+
+// Type annotations.
+void ft(int __attribute__((acquire_handle)) * a);
+void (*fpt)(int __attribute__((use_handle)) handle);
+auto lambdat = [](int __attribute__((use_handle)) handle) ->
+int __attribute__((acquire_handle)) { return 0; };
+void gt(int __attribute__((acquire_handle)) a); // expected-error {{attribute only applies to output parameters}}
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error {{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning {{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, parameters, and typedefs}}
+
+// Typedefs.
+using out_handle = int __attribute__((acquire_handle)) *;
+typedef int __attribute__((release_handle)) close_handle;
+int replace_handle(close_handle handle, out_handle hanlde2);
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_hasType_functionType, SubjectMatchRule_variable_is_parameter, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Alias (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: AlignValue (SubjectMatchRule_variable, SubjectMatchRule_type_alias)
 // CHECK-NEXT: AllocSize (SubjectMatchRule_function)
@@ -128,6 +129,7 @@
 // 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, SubjectMatchRule_type_alias)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
@@ -145,6 +147,7 @@
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
+// CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter, SubjectMatchRule_type_alias)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7418,6 +7418,21 @@
  attrKind == ParsedAttr::AT_OpenCLGenericAddressSpace;
 }
 
+template 
+static void handleHandleAttr(TypeProcessingState , QualType ,
+ ParsedAttr ) {
+  if 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

aaron.ballman wrote:
> aaron.ballman wrote:
> > Is this correct, or should it be `if (!CurType->isFunctionType())`?
> I see now that this is only incorrect for the acquire attribute, but not the 
> other two.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

aaron.ballman wrote:
> Is this correct, or should it be `if (!CurType->isFunctionType())`?
I see now that this is only incorrect for the acquire attribute, but not the 
other two.



Comment at: clang/test/Sema/attr-handles.cpp:20-21
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error 
{{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning 
{{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only 
applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' 
attribute only applies to functions, function pointers, and parameters}}

xazax.hun wrote:
> aaron.ballman wrote:
> > 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.
> 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. 
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
```



Comment at: clang/test/Sema/attr-handles.cpp:3
+
+// Decl annotations.
+void f(int *a __attribute__((acquire_handle)));

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232723.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+// Decl annotations.
+void f(int *a __attribute__((acquire_handle)));
+void (*fp)(int handle [[clang::use_handle]]);
+auto lambda = [](int handle [[clang::use_handle]]){};
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle(1; // expected-error {{'acquire_handle' attribute takes no arguments}}
+__attribute__((release_handle)) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle)) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle)); // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
+
+// Type annotations.
+void ft(int __attribute__((acquire_handle)) * a);
+void (*fpt)(int __attribute__((use_handle)) handle);
+auto lambdat = [](int __attribute__((use_handle)) handle) ->
+int __attribute__((acquire_handle)) { return 0; };
+void gt(int __attribute__((acquire_handle)) a); // expected-error {{attribute only applies to output parameters}}
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error {{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning {{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_hasType_functionType, 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)
@@ -128,6 +129,7 @@
 // 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)
@@ -145,6 +147,7 @@
 // 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)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7418,6 +7418,21 @@
  attrKind == ParsedAttr::AT_OpenCLGenericAddressSpace;
 }
 
+template 
+static void handleHandleAttr(TypeProcessingState , QualType ,
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)
+<< Attr.getAttrName()->getName() << 3 << CurType;
+Attr.setInvalid();
+return;
+  }
+  ASTContext  = State.getSema().Context;
+  CurType = State.getAttributedType(createSimpleAttr(Ctx, Attr),
+CurType, CurType);
+  Attr.setUsedAsTypeAttr();
+}
+
 static void 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added inline comments.



Comment at: clang/test/Sema/attr-handles.cpp:20-21
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error 
{{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning 
{{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only 
applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' 
attribute only applies to functions, function pointers, and parameters}}

aaron.ballman wrote:
> 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.
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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7424
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)

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



Comment at: clang/test/Sema/attr-handles.cpp:20-21
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error 
{{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning 
{{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only 
applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' 
attribute only applies to functions, function pointers, and parameters}}

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.



Comment at: clang/test/Sema/attr-handles.cpp:23
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' 
attribute only applies to functions, function pointers, and parameters}}
\ No newline at end of file


Please add the newline to the end of the file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232681.
xazax.hun marked 7 inline comments as done.
xazax.hun added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+// Decl annotations.
+void f(int *a __attribute__((acquire_handle)));
+void (*fp)(int handle [[clang::use_handle]]);
+auto lambda = [](int handle [[clang::use_handle]]){};
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle(1; // expected-error {{'acquire_handle' attribute takes no arguments}}
+__attribute__((release_handle)) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle)) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle)); // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
+
+// Type annotations.
+void ft(int __attribute__((acquire_handle)) * a);
+void (*fpt)(int __attribute__((use_handle)) handle);
+auto lambdat = [](int __attribute__((use_handle)) handle) ->
+int __attribute__((acquire_handle)) { return 0; };
+void gt(int __attribute__((acquire_handle)) a); // expected-error {{attribute only applies to output parameters}}
+void ht(int __attribute__((acquire_handle(1))) *a); // expected-error {{'acquire_handle' attribute takes no arguments}}
+int it() __attribute__((release_handle)); // expected-warning {{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int jt() __attribute__((use_handle)); // expected-warning {{'use_handle' only applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) at; // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
\ No newline at end of file
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_hasType_functionType, 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)
@@ -128,6 +129,7 @@
 // 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)
@@ -145,6 +147,7 @@
 // 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)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7418,6 +7418,21 @@
  attrKind == ParsedAttr::AT_OpenCLGenericAddressSpace;
 }
 
+template 
+static void handleHandleAttr(TypeProcessingState , QualType ,
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(), diag::warn_type_attribute_wrong_type)
+<< Attr.getAttrName()->getName() << 3 << CurType;
+ 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7412
+ diag::warn_type_attribute_wrong_type_str)
+<< Attr.getAttrName()->getName() << "non-function" << CurType;
+return;

aaron.ballman wrote:
> 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.
Indeed. But only passing Attr would result in duplicated `'`. I'm reusing the 
message, so I ended up not changing this part.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang/include/clang/Basic/Attr.td:3464
+
+def AcquireHandle : TypeAttr {
+  let Spellings = [Clang<"acquire_handle">];

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



Comment at: clang/include/clang/Basic/AttrDocs.td:4569
+
+def HandleDocs : DocumentationCategory<"Handle Attributes"> {
+  let Content = [{

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



Comment at: clang/lib/Sema/SemaType.cpp:7412
+ diag::warn_type_attribute_wrong_type_str)
+<< Attr.getAttrName()->getName() << "non-function" << CurType;
+return;

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.



Comment at: clang/test/Sema/attr-handles.cpp:7
+int __attribute__((acquire_handle)) { return 0; };
+void g(int __attribute__((acquire_handle)) a); // TODO: diagnose this. The 
acquire attribute only makes sense for outputs.
+void h(int __attribute__((acquire_handle(1))) *a); // expected-error 
{{'acquire_handle' attribute takes no arguments}}

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

@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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 231784.
xazax.hun added a comment.

- Convert to type attributes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+void f(int __attribute__((acquire_handle)) * a);
+void (*fp)(int __attribute__((use_handle)) handle);
+auto lambda = [](int __attribute__((use_handle)) handle) -> 
+int __attribute__((acquire_handle)) { return 0; };
+void g(int __attribute__((acquire_handle)) a); // TODO: diagnose this. The acquire attribute only makes sense for outputs.
+void h(int __attribute__((acquire_handle(1))) *a); // expected-error {{'acquire_handle' attribute takes no arguments}}
+int i() __attribute__((release_handle)); // expected-warning {{'release_handle' only applies to non-function types; type here is 'int ()'}}
+int j() __attribute__((use_handle)); // expected-warning {{'use_handle' only applies to non-function types; type here is 'int ()'}}
+int __attribute__((acquire_handle)) a; // TODO: diagnose this. The type attribute only makes sense for function parameters, return types, type aliases.
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7403,6 +7403,20 @@
   }
 }
 
+template 
+static void handleHandleAttr(TypeProcessingState , QualType ,
+ ParsedAttr ) {
+  if (CurType->isFunctionType()) {
+State.getSema().Diag(Attr.getLoc(),
+ diag::warn_type_attribute_wrong_type_str)
+<< Attr.getAttrName()->getName() << "non-function" << CurType;
+return;
+  }
+  ASTContext  = State.getSema().Context;
+  CurType = State.getAttributedType(createSimpleAttr(Ctx, Attr),
+CurType, CurType);
+  Attr.setUsedAsTypeAttr();
+}
 
 static void processTypeAttrs(TypeProcessingState , QualType ,
  TypeAttrLocation TAL,
@@ -7600,6 +7614,15 @@
   else if (!handleFunctionTypeAttr(state, attr, type))
 distributeFunctionTypeAttr(state, attr, type);
   break;
+case ParsedAttr::AT_UseHandle:
+  handleHandleAttr(state, type, attr);
+  break;
+case ParsedAttr::AT_AcquireHandle:
+  handleHandleAttr(state, type, attr);
+  break;
+case ParsedAttr::AT_ReleaseHandle:
+  handleHandleAttr(state, type, attr);
+  break;
 }
 
 // Handle attributes that are defined in a macro. We do not want this to be
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3042,6 +3042,9 @@
   "'%0' only applies to %select{function|pointer|"
   "Objective-C object or block pointer}1 types; type here is %2">,
   InGroup;
+def warn_type_attribute_wrong_type_str : Warning<
+  "'%0' only applies to %1 types; type here is %2">,
+  InGroup;
 def warn_incomplete_encoded_type : Warning<
   "encoding of %0 type is incomplete because %1 component has unknown encoding">,
   InGroup>;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -4565,3 +4565,62 @@
   }
   }];
 }
+
+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 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 __attribute__((acquire_handle))* out1);
+
+
+  // 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

xazax.hun wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > What about function-like interfaces such as lambdas, blocks, or other 
> > > > > callable objects that are not a `FunctionDecl`?
> > > > Good point!
> > > 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).
> > 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?
> 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.
> 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.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6524
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)

xazax.hun wrote:
> aaron.ballman wrote:
> > 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?
> 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?
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > What about function-like interfaces such as lambdas, blocks, or other 
> > > callable objects that are not a `FunctionDecl`?
> > Good point!
> 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).
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?



Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > What about function-like interfaces such as lambdas, blocks, or other 
> > > > callable objects that are not a `FunctionDecl`?
> > > Good point!
> > 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).
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 231307.
xazax.hun marked 5 inline comments as done.
xazax.hun added a comment.

- Address review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.cpp

Index: clang/test/Sema/attr-handles.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-handles.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+void f(int *a __attribute__((acquire_handle)));
+void (*fp)(int handle [[clang::use_handle]]);
+auto lambda = [](int handle [[clang::use_handle]]){};
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle(1; // expected-error {{'acquire_handle' attribute takes no arguments}}
+__attribute__((release_handle)) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle)) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle)); // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_hasType_functionType, 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)
@@ -128,6 +129,7 @@
 // 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)
@@ -145,6 +147,7 @@
 // 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)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6540,6 +6540,18 @@
   handleSimpleAttribute(S, D, AL);
 }
 
+static void handeAcquireHandleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // Warn if the parameter is definitely not an output parameter.
+  if (const auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
+  << AL.getRange();
+  return;
+}
+  }
+  handleSimpleAttribute(S, D, AL);
+}
+
 //===--===//
 // Top Level Sema Entry Points
 //===--===//
@@ -7311,6 +7323,16 @@
   case ParsedAttr::AT_ArmMveAlias:
 handleArmMveAliasAttr(S, D, AL);
 break;
+
+  case ParsedAttr::AT_AcquireHandle:
+handeAcquireHandleAttr(S, D, AL);
+break;
+  case ParsedAttr::AT_ReleaseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_UseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3101,6 +3101,8 @@
 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">;
 
 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6524
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)

aaron.ballman wrote:
> 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?
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

xazax.hun wrote:
> aaron.ballman wrote:
> > What about function-like interfaces such as lambdas, blocks, or other 
> > callable objects that are not a `FunctionDecl`?
> Good point!
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).



Comment at: clang/include/clang/Basic/Attr.td:3451
+  let Spellings = [Clang<"use_handle">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [UseHandleDocs];

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?



Comment at: clang/include/clang/Basic/AttrDocs.td:4473
+  let Content = [{
+Handles are a way to identify resources like files, sockets, processes. They
+are more opaque than pointers and widely used in system programming. They have

sockets, processes -> sockets, and processes



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6523
+  // Warn if the parameter is definitely not an output parameter.
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {

`const auto *`



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6524
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 230347.
xazax.hun marked 8 inline comments as done.
xazax.hun added a comment.

- Address review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-handles.c

Index: clang/test/Sema/attr-handles.c
===
--- /dev/null
+++ clang/test/Sema/attr-handles.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+void f(int *a __attribute__((acquire_handle)));
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
+void h(int *a __attribute__((acquire_handle(1; // expected-error {{'acquire_handle' attribute takes no arguments}}
+__attribute__((release_handle)) int i(); // expected-warning {{'release_handle' attribute only applies to parameters}}
+__attribute__((use_handle)) int j(); // expected-warning {{'use_handle' attribute only applies to parameters}}
+int a __attribute__((acquire_handle)); // expected-warning {{'acquire_handle' attribute only applies to functions, function pointers, and parameters}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_hasType_functionType, 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)
@@ -126,6 +127,7 @@
 // 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)
@@ -143,6 +145,7 @@
 // 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)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6518,6 +6518,18 @@
   handleSimpleAttribute(S, D, AL);
 }
 
+static void handeAcquireHandleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // Warn if the parameter is definitely not an output parameter.
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
+  << AL.getRange();
+  return;
+}
+  }
+  handleSimpleAttribute(S, D, AL);
+}
+
 //===--===//
 // Top Level Sema Entry Points
 //===--===//
@@ -7282,6 +7294,16 @@
   case ParsedAttr::AT_ArmMveAlias:
 handleArmMveAliasAttr(S, D, AL);
 break;
+
+  case ParsedAttr::AT_AcquireHandle:
+handeAcquireHandleAttr(S, D, AL);
+break;
+  case ParsedAttr::AT_ReleaseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_UseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3072,6 +3072,8 @@
 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;
Index: 

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3450
+def UseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"use_handle">];
+  let Documentation = [UseHandleDocs];

xazax.hun wrote:
> aaron.ballman wrote:
> > Should this have a subject limiting it to parameters?
> My understanding was that the subject list is "inherited" from 
> InheritableParamAttr, which already has it limited to parameters. Is this not 
> the case?
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.



Comment at: clang/include/clang/Basic/AttrDocs.td:4474
+  let Content = [{
+If this annotation is on a function it is assumed to return a new handle.
+In case this annotation is on an output parameter, the function is assumed

xazax.hun wrote:
> aaron.ballman wrote:
> > What is a "handle"? I think some introduction docs are needed.
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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




Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

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



Comment at: clang/include/clang/Basic/Attr.td:3450
+def UseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"use_handle">];
+  let Documentation = [UseHandleDocs];

Should this have a subject limiting it to parameters?



Comment at: clang/include/clang/Basic/Attr.td:3455
+def ReleaseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"release_handle">];
+  let Documentation = [ReleaseHandleDocs];

Should this have a subject limiting it to parameters?



Comment at: clang/include/clang/Basic/AttrDocs.td:4474
+  let Content = [{
+If this annotation is on a function it is assumed to return a new handle.
+In case this annotation is on an output parameter, the function is assumed

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 230294.
xazax.hun added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-acquire_handle.c

Index: clang/test/Sema/attr-acquire_handle.c
===
--- /dev/null
+++ clang/test/Sema/attr-acquire_handle.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+void f(int *a __attribute__((acquire_handle)));
+void g(int a __attribute__((acquire_handle))); // expected-error {{attribute only applies to output parameters}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_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)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6518,6 +6518,18 @@
   handleSimpleAttribute(S, D, AL);
 }
 
+static void handeAcquireHandleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // Warn if the parameter is definitely not an output parameter.
+  if (auto *PVD = dyn_cast(D)) {
+if (PVD->getType()->isIntegerType()) {
+  S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
+  << AL.getRange();
+  return;
+}
+  }
+  handleSimpleAttribute(S, D, AL);
+}
+
 //===--===//
 // Top Level Sema Entry Points
 //===--===//
@@ -7282,6 +7294,16 @@
   case ParsedAttr::AT_ArmMveAlias:
 handleArmMveAliasAttr(S, D, AL);
 break;
+
+  case ParsedAttr::AT_AcquireHandle:
+handeAcquireHandleAttr(S, D, AL);
+break;
+  case ParsedAttr::AT_ReleaseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_UseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3072,6 +3072,8 @@
 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;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -4467,3 +4467,50 @@
   }
   }];
 }
+
+def AcquireHandleDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+If this annotation is on a function 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* out0 [[clang::acquire_handle]],
+   zx_handle_t* out1 [[clang::acquire_handle]]);
+
+
+  // Returned handle.
+  [[clang::acquire_handle]] int open(const char *path, int oflag, ... );
+  }];
+}
+
+def UseHandleDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+A function taking a handle by value might close the handle. If a function 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,
+   

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 3 inline comments as done.
xazax.hun added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];

aaron.ballman wrote:
> What about function-like interfaces such as lambdas, blocks, or other 
> callable objects that are not a `FunctionDecl`?
Good point!



Comment at: clang/include/clang/Basic/Attr.td:3450
+def UseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"use_handle">];
+  let Documentation = [UseHandleDocs];

aaron.ballman wrote:
> Should this have a subject limiting it to parameters?
My understanding was that the subject list is "inherited" from 
InheritableParamAttr, which already has it limited to parameters. Is this not 
the case?



Comment at: clang/include/clang/Basic/AttrDocs.td:4474
+  let Content = [{
+If this annotation is on a function it is assumed to return a new handle.
+In case this annotation is on an output parameter, the function is assumed

aaron.ballman wrote:
> What is a "handle"? I think some introduction docs are needed.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287
+  case ParsedAttr::AT_AcquireHandle:
+handleSimpleAttribute(S, D, AL);
+break;

NoQ wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > 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).
> > 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. 
> 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?
Hmm, I think this makes a lot of sense! So basically, I would exclude 
primitive, non-pointer types that are taken by value. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287
+  case ParsedAttr::AT_AcquireHandle:
+handleSimpleAttribute(S, D, AL);
+break;

xazax.hun wrote:
> NoQ wrote:
> > 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).
> 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. 
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287
+  case ParsedAttr::AT_AcquireHandle:
+handleSimpleAttribute(S, D, AL);
+break;

NoQ wrote:
> 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).
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287
+  case ParsedAttr::AT_AcquireHandle:
+handleSimpleAttribute(S, D, AL);
+break;

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70469/new/

https://reviews.llvm.org/D70469



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: aaron.ballman, NoQ.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70469

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -9,6 +9,7 @@
 // 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_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)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -7282,6 +7282,16 @@
   case ParsedAttr::AT_ArmMveAlias:
 handleArmMveAliasAttr(S, D, AL);
 break;
+
+  case ParsedAttr::AT_AcquireHandle:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_ReleaseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_UseHandle:
+handleSimpleAttribute(S, D, AL);
+break;
   }
 }
 
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -4467,3 +4467,50 @@
   }
   }];
 }
+
+def AcquireHandleDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+If this annotation is on a function 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* out0 [[clang::acquire_handle]],
+   zx_handle_t* out1 [[clang::acquire_handle]]);
+
+
+  // Returned handle.
+  [[clang::acquire_handle]] int open(const char *path, int oflag, ... );
+  }];
+}
+
+def UseHandleDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+A function taking a handle by value might close the handle. If a function 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 = DocCatDecl;
+  let Content = [{
+If a function 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]]);
+  }];
+}
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -3439,3 +3439,19 @@
   let Subjects = SubjectList<[Function]>;
   let Documentation = [NoBuiltinDocs];
 }
+
+def AcquireHandle : InheritableAttr {
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];
+}
+
+def UseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"use_handle">];
+  let Documentation = [UseHandleDocs];
+}
+
+def ReleaseHandle : InheritableParamAttr {
+  let Spellings = [Clang<"release_handle">];
+  let Documentation = [ReleaseHandleDocs];
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits