[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)
Timm =?utf-8?q?Bäder?= , Timm =?utf-8?q?Bäder?= , Timm =?utf-8?q?Bäder?= , Timm =?utf-8?q?Bäder?= , Timm =?utf-8?q?Bäder?= Message-ID: In-Reply-To: kevmw wrote: > This example from the failing test should warn I think > [...] > Since the definition of foo2 specifies attributes that aren't present in the > previous declaration. Yes, this looks exactly like the thing we want to protect against to me (because chances are that the declaration is actually in a header file and other source files will check their calls only against this declaration, so real bugs can be expected to result from this). https://github.com/llvm/llvm-project/pull/67520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
