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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D92039#2457867 <https://reviews.llvm.org/D92039#2457867>, @aaron.ballman wrote:

> Is the attribute considered to be a property of the parameter or a property of the function the parameter is declared in? e.g.,
>
>   void someOtherFunc(void (^cb)(void)) {
>     if (something())
>       cb();
>   }
>   
>   void barWithCallback(void (^callback)(void) __attribute__((called_once))) {
>     someOtherFunc(callback);
>   }
>
> Should code like that also diagnose given that `callback` is not called on every code path? 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. This in turn makes me wonder whether a better design is for `-Wcompletion-handler` to diagnose any function with a completion handler-like parameter if the parameter is not called exactly once within the function, and the user marks the functions that should be exempt from the checking rather than marking the parameters that should be checked (or does this have too many false positives in practice)? Alternatively, should the check be implemented as a clang static analyzer check that tracks an annotated parameter across the inter-procedural CFG and diagnoses such code?



> should the check be implemented as a clang static analyzer check that tracks an annotated parameter across the inter-procedural CFG and diagnoses such code?

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.

> 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.  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.

> 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)))`.



================
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.
+
----------------
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.


================
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.
----------------
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}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92039