[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<ParmVarDecl>(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