[clang] Thread Safety Analysis: Support attributes on function pointers (PR #191187)

[Marco Elver via cfe-commits]

melver wrote:


> I think this is important functionality, but that's why I don't think it makes sense to rush landing it.

Thanks for the review!

> > Clang parses this as an attribute to the declarator (the variable/field being declared), which is exactly what we want - if it were a type attribute (or qualifier even), it would have to be nested deep inside the pointer and function syntax, which would be rather horrible to read.
> 
> That depends on the _syntax_ used. With the `__attribute__` spelling, you are correct. But with the `[[]]` spelling, that applies to the function type. And making this a property of the function type seems like a pretty reasonable idea: it means any calls through the function pointer can be guaranteed to have the acquire/release/etc behaviors and any function declarations which do not meet that guarantee fail to compile due to type incompatibility.
> 
> Also, I don't see why you think it would make for hard-to-read syntax.

Ack. Though note, all users of TSA I'm aware of use the `__attribute__` spelling, incl. our own C++ tests (I added a test for the `[[]]` spelling).

> ```
> void foo() [[clang::acquire_capability(whatever)]];
> void bar();
> void baz() [[clang::acquire_capability(something_else)]];
> 
> using acquire_fp = void (*)() [[clang::acquire_capability(whatever)]];
> 
> acquire_fp works = foo; // Ok
> acquire_fp fails1 = bar; // error: incompatible types due to incompatible attributes
> acquire_fp fails2 = baz; // error: incompatible types due to incompatible attribute arguments
> ```
> 
> That said, I don't insist on going that route, there's a lot of complexity to consider. But the fact that we document this:
> 
> > Assigning a function with different (or no) attributes to an
> > annotated function pointer variable is not diagnosed.  The analysis trusts the
> > annotations on the variable at the call site.
> 
> suggests we should be pursuing the type attribute. This is basically "we have a type system which could point out problems but we decided not to use it"; we can understand the design tradeoffs as compiler engineers, but from a safety analysis perspective it seems like a questionable tradeoff.

If we could create TSA2, I agree on the merits - but in my view it's a very intrusive change that will affect every existing user and likely result in lots of broken builds (e.g. assigning annotated function to unannotated function pointer - works now, would break if we changed).

In general, I still think the declaration route is the right trade-off, despite sacrificing on soundness a bit:
1. Every TSA usage today is a declaration-level annotation, not a type-level one. Changing that causes a lot of friction, and I don't see an incremental path to transition to that design without creating TSA2.
2. #67095 attempted the type-attribute route and remains unresolved after several years. Besides my fundamental concern in (1),  its scope (`FunctionType::ExtInfo` changes, etc.) is significantly   larger than this PR.

What we gain is additional coverage, reducing false positives (only alternative before was `no_thread_safety_analysis`), without regressing existing codebases.

> Some additional test coverage I think may help is:
> 
> * Testing more edge cases, like arrays of annotated function pointers, pointer/reference to array of annotated function pointers.

Added - added test for arrays, which is current rejected with an error.

> * Testing `[[]]` vs `__attribute__` behavioral differences to ensure that `[[]]` spellings follow the correct appertainment rules on type vs declaration behavior

Added a `[[]]` test (placed at front of declaration, given we need it to appertain to declared variable).

https://github.com/llvm/llvm-project/pull/191187