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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> That was a tough choice whether we should do it in the analyzer or not.
> The analyzer check would've been easier in terms of existing infrastructure, path sensitivity, and IPA.  It is also much more lenient in terms of false positives (it is expected that the analyzer can have those).
> However, warnings have much broader audience and we aim for a higher numbers of people to check their code (we've been asked to deliver it for 8 years).  And because the analyzer doesn't have cross-translation unit analysis, the problem with inter procedural bugs appears as well.

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.

>> From the test cases you have, it looks like you explicitly allow this code without diagnosing it, which suggests the attribute is really a property of the function and not a property of the parameter.
>
> I have to disagree here.  //Semantically// this is a property of a parameter and the fact that we couldn't analyze inter-procedurally is our own problem and implementation detail.

Okay, that's good to know that you think the semantics should follow the parameter rather than the function. I agree that the analysis part is our own problem, I'm mostly trying to make sure we add this functionality to the correct place within the toolchain.

> We don't want to bother users with this.  As far as I understand, the majority of existing warnings are not complete (don't find every bug) and this warning is not different.

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?

>> and the user marks the functions that should be exempt from the checking rather than marking the parameters that should be checked
>
> Essentially, there is a way to prevent function from being analyzed by `-Wcompletion-handler` - annotate it with `__attribute__((swift_async(none)))`.

Ah, that's good to know, thanks!



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5133-5134
+
+This attribute is useful for API developers who want to double-check if they
+implemented their method correctly.
+
----------------
vsavchenko wrote:
> aaron.ballman wrote:
> > Oh, I was thinking this attribute was enabling some optimization opportunities or doing something more than just acting as a marker to please check this function's correctness for some properties. This means that the programmer has to annotate their code and enable the warning flag in order for either the attribute or the warning flag to have effect, which feels a bit surprising to me.
> For explicitly marked parameters, it's on by default, so the users can simply annotate their parameters and get their checks.
Ah, yes, there are two diagnostics being used and only one of them is off by default. Thank you for clarifying.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5098-5102
+* Parameter is not called at all.
+
+* Parameter is called more than once.
+
+* Parameter is not called on one of the execution paths.
----------------
vsavchenko wrote:
> aaron.ballman wrote:
> > I think you may have to remove the newlines here to make this a single bulleted list in Sphinx.
> I checked it, it is one list with newlines: {F14734606}
Oh, good to know, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92039