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
