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

[Kevin Wolf via cfe-commits]

kevmw wrote:

> But as a rule of thumb, static analysis attributes always belong on the publicly visible declaration. Otherwise, the caller can't see them.

Yes, the theory is obvious and easy to understand, at least once you think of it. Applying it in practice is where I made the experience that mistakes happen a bit too easily.

When you have a mix of static and public functions (so sometimes the annotations are on the definition and sometimes in the header), you're bound to miss a missing "static" sooner or later even if you know the theory. Being careful only gets you so far, so it would be really nice to get that support from the compiler. (And after all, TSA is all about getting support from the compiler instead of trying to be careful yourself.)

Another way to deal with the mistakes would be just outright forbidding most TSA attributes on definitions for public functions, though that's even more restrictive ("TSA attributes on the definition have to be empty" vs. "TSA attributes on the definition have to be a subset of the attributes on declarations").

What I think would make me prefer having the annotations in both places is that when editing a source file, it is slightly inconvenient to have to look at the header to know which assumptions it can make, and it's also slightly inconvenient to update a function in both places if you can't just copy the line, but they have to be different. But that's merely a question of convenience, so if forbidding works better for you, I could live with that, too.

> Ok, that's an interesting twist. I could live with that, but I'm not sure if we handle any other atttributes this way.

Yeah, I'm not sure if there are any that work the exact same way (though I also didn't actively look for one). But as @AaronBallman said above, the attributes are additive, so comparing the definition with the final set of TSA annotations of all declarations combined – which is the same as checking for "at least one" – would seem to make a lot of sense to me.

But as I concluded, checking all previous declarations for function definitions (and only for definitions) should actually work fine in practice, too, unless you're doing crazy things like writing your own implementation with TSA annotations while using a third-party header file for it that doesn't have them. In which case you should probably just copy that header file instead, or locally turn off the warning for that function.

I also compared TSA annotations to calling conventions. There it's possible to have the attribute only on the first declaration and leave it out on later ones (but not the other way around, and obviously also not specifying two different calling conventions). Modelling it after that should work, too, though I'm not sure if the consistency with another attribute is worth the additional limitation.

So there are options and the exact mode is for you guys to decide, I don't mind too much either way.

> What I was thinking about was warning on any redeclaration that's adding attributes. Sure, if you only do this on definitions, adding attributes to third-party library wouldn't be an issue.

Ah, I see. Maybe I misunderstood the code of this PR, but I thought it only checks definitions. And that's honestly all I think we need. It will always be possible to fool TSA with declarations that one file sees and another doesn't, but if we can warn in the common case of a function with one declaration in a header and a definition in a specific source file, it would probably catch 99% of cases in practice.

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