[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)

[Kevin Wolf via cfe-commits]

kevmw wrote:

I'm the person who asked @tbaederr last year if this could be added for the benefit of QEMU, and as I'm looking at it again now, I thought maybe I should leave a comment here.

In QEMU, we currently handle the problem with a coding convention (public functions get TSA attributes _only_ in the header) and rely on manual code review that the convention is actually obeyed. Obviously, having something automatable like this handled by the compiler would be much nicer and less error prone, so we would still like to see this implemented eventually in some form.

> I could imagine this being useful for RequiresCapability in cases where you only want to require the capability internally as an implementation detail, but not make it part of the public interface of the function.

Not sure what scenario you have in mind there. How is requiring a capability ever something internal? Aren't required capabilities by definition part of the contract of a function?

For example, if a function uses RequiresCapability to specify that it must be called with some lock held and you call it without that lock, that is a bug. If the function takes a lock only internally, then it wouldn't specify that in a TSA attribute because these attributes are only about the interface of the function.

I would actually compare this to function declarations with conflicting calling conventions. This is always a hard compiler error – calling the function with a different calling convention would be a bug, just like calling a function without the right capabilities. (Though admittedly, differing calling conventions would typically result in more obvious bugs if the compiler didn't catch them...)

Am I missing some legitimate scenario in which your code snippet shouldn't cause a warning here because I'm primarily thinking of capabilities as used with locks?

> Yeah, it's a tricky question. On one hand there have been previous issues like #42194 (which this would basically address), and it would definitely improve consistency and prevent beginner's mistakes. On the other hand it seems useful to allow adding attributes e.g. to third-party libraries where you don't control the headers.

I'm not sure that this mistake is one that only beginners would make.

But I agree that adding attributes to functions from third-party libraries is important. I think this is why I first suggested the following in the [downstream tracker](https://issues.redhat.com/browse/RHEL-7269): "I propose that clang should warn if a function definition has TSA attributes, a previous declaration of the same function exists and the attribute isn't present on **at least one** of the previous declarations."

But come to think of it, I'm not sure any more if this really makes a difference – typically, you don't even have a definition for functions from third-party libraries that could trigger the warning, only declarations. The exception are inline functions in headers, but even then, what you get is a declaration with the attribute and a definition without it, which still wouldn't trigger the warning. So do we actually have a problem here?

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