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

Wed Dec 11 12:46:24 PST 2019

NoQ added inline comments.

================
Comment at: clang/lib/Sema/SemaType.cpp:7424
+                             ParsedAttr &Attr) {
+  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(&handle); // What will this do?
> ```
> 
> The function `f` would be free to do anything, like store the address of the handle in a global data structure, overwrite the closed handle and so on. The purpose of the `use` annotation is to tell the analyzer that none of this will happen, so we can safely diagnose a use after release. So the original idea was to annotate those functions that are guaranteed to never close, overwrite etc the handle.
> For unannotated functions the analyzer will be conservative and assume that the code is OK. 
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