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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D70469#1766084 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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