[PATCH] D92039: [-Wcalled-once-parameter] Introduce 'called_once' attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D92039#2462255 <https://reviews.llvm.org/D92039#2462255>, @vsavchenko wrote:

> In D92039#2458392 <https://reviews.llvm.org/D92039#2458392>, @aaron.ballman wrote:
>
>> 
>
>
>
>> There have been efforts to add cross-TU support to the static analyzer, so I'm less worried about cross-TU inter-procedural bugs over the long term as I would expect that situation to be improved.
>
> I'm a bit less optimistic on that front, the whole model of how we do the analysis should be redesigned in order to get true cross-TU inter-procedural analysis.  Additionally, some checkers when essentially inter-procedural rely on function conventions to assume behavior of called functions, the same way it is done with this analysis.
>
>> I think my concern is somewhat different. If the goal is for the semantics to follow the parameter (which I also think is the correct behavior), then I think adding this as a frontend analysis is the incorrect place for the check to go because someday we're going to want the inter-procedural analysis and that will require us to figure out what to do with the frontend bits vs the static analyzer bits. If it starts off in the static analyzer, then the later improvements to the analysis capabilities don't require the user to change the way they interact with the toolchain.
>>
>> Or do you expect that this analysis is sufficient and you don't expect to ever extend it to be inter-procedural?
>
> As I noted above, in some sense it is already inter-procedural because usually called functions follow completion handler conventions as well.

Your test cases suggest that this is not inter-procedurally checked; it seems to require all of the APIs to be annotated in order to get the same effect that an inter-procedural check could determine on its own.

> So, we can actually make it inter-procedural if all involved functions follow conventions. 
> And we can warn people if they leak `called_once` parameter into a function that doesn't specify what it will do with that parameter.
>
> It was a decision not to do this and to be more lenient in the first version.  You can see that in a lot of cases during the analysis we assume that the user knows what they are doing.  And even in this model the number of reported warnings is pretty high.
> We hope that this warning will help to change those cases and, maybe in the future, we can harden the analysis to be more demanding.

I'm not worried about the leniency and I'm sorry if it sounds like I'm implying that it has to be inter-procedural to land -- that's not the case. What I am worried about is that the way the user runs the frontend is very different than the way the user runs the static analyzer, and migrating the warning from the FE to the static analyzer could leave us with problems. If the intention is that this check will live in the FE as-is (not getting inter-procedural support), then that's fine. But if the intention is to extend the check in the future in a way that might need it to move out of the FE and into the CSA, then I think the check should live in the CSA from the start (even if it doesn't do inter-procedural analysis). It's not clear to me whether "in the first version" implies that you expect a subsequent version to live in the CSA or not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92039/new/

https://reviews.llvm.org/D92039