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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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

> Add one gigantic comment on status kinds and the reasons behind some of the design choices

This was freakin' awesome, thanks a lot. With all the background in place, the rest of the patch was a really nice read. I expected it to be a lot more hacky but it turned out very sane to be honest. I only have minor remarks here and there.



================
Comment at: clang/include/clang/Analysis/Analyses/CalledOnceCheck.h:32
+/// \enum SwitchSkipped -- there is no call if none of the cases appies.
+/// \enum LoopEntered -- no call when the loop is entered.
+/// \enum LoopSkipped -- no call when the loop is not entered.
----------------
Hold up, how can this happen at all without path sensitivity? If the loop is not entered then literally nothing happens, so there can't be a call. If there's no call in either case then probably it's just "there's simply no call at all"?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:67
+  // One of the erroneous situations is the case when parameter is called only
+  // on some of the paths.  We couldv'e considered it `NotCalled`, but we want
+  // to report double call warnings even if these two calls are not guaranteed
----------------



================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:83-84
+  //
+  // Our analysis is intra-procedural and, while in the perfect world,
+  // developers only use tracked parameters to call them, in the real world,
+  // the picture might be different.  Parameters can be stored in global
----------------
I think there's nothing wrong with / shameful about escaping, so i wouldn't call the world without escapes a perfect world, it's unnecessarily constrained :)


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:163
+  }
+  static bool canBeCalled(Kind K) {
+    return (K & DefinitelyCalled) == DefinitelyCalled && K != Reported;
----------------
"Can be called" sounds like "Someone's allowed to call it". I was completely confused when i saw a call site for that function.

I too am at loss because the concept of "is definitely potentially called" is hard to explain especially when the concept of "maybe called" is already defined and means something different.

Maybe `seenAnyCalls()` or something like that?

Maybe also rename `DefinitelyCalled` and `MaybeCalled` to `CalledOnAllPaths` and `CalledOnSomePathsOnly`? That could eliminate a bit more confusion.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:304-307
+    // Function pointer/references can be dereferenced before a call.
+    // That doesn't make it, however, any different from a regular call.
+    // For this reason, dereference operation is a "no-op".
+    case UO_Deref:
----------------
Technically the same applies to operator `&`, eg. `(&*foo)()` is your normal call. Obviously, `(&foo)()` doesn't compile. That said, `if (&foo)` compiles and is not your normal null check (but `if (&*foo)` and `if (*foo)` are).


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:321-322
+    case BO_NE: {
+      const DeclRefExpr *LHS = Visit(BO->getLHS());
+      return LHS ? LHS : Visit(BO->getRHS());
+    }
----------------
You're potentially dropping some `DeclRefExpr`s. Also the ones that you find aren't necessarily the function pointer variables you're looking for (as you don't actually have any checks in `VisitDeclRefExpr`). Combined, i suspect that there may be some exotic counterexamples like
```lang=c
if (!foo) {
  // "foo" is found
  if (foo == completion_handler) {
    // ...
  }
}
```
(which is probably ok anyway)


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:541-542
+private:
+  NotCalledClarifier(const CFGBlock *Parent, const CFGBlock *SuccInQuestion)
+      : Parent(Parent), SuccInQuestion(SuccInQuestion) {}
+
----------------
This is one of the more subtle facilities, i really want a comment here a lot more than on the `NamesCollector`. Like, which blocks do you feed into this class? Do i understand correctly that `Parent` is the block at which we decided to emit the warning? And `SuccInQuestion` is its successor on which there was no call? Or on which there was a call? I can probably ultimately figure this out if i read all the code but i would be much happier if there was a comment.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:560
+                    bool CheckConventionalParameters)
+      : FunctionCFG(FunctionCFG), AC(AC), Handler(Handler),
+        CheckConventionalParameters(CheckConventionalParameters),
----------------
Why not `FunctionCFG(AC->getCFG())` and omit the CFG parameter? That's the same function, right?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:780
+    //
+    // The following algorithm has the worst case complexity of O(N + E),
+    // where N is the number of basic blocks in FunctionCFG,
----------------
I'm super used to `V` and `E` for Vertices and Edges in the big-O-notation for graph algorithms, is it just me?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:822
+  /// calling the parameter itself, but rather uses it as the argument.
+  template <class CallLikeExpr>
+  void checkIndirectCall(const CallLikeExpr *CallOrMessage) {
----------------
Did you consider `AnyCall`? That's a universal wrapper for all kinds of AST calls for exactly these cases. It's not super compile-time but it adds a lot of convenience. (Also uh-oh, its doxygen page seems to be broken). It's ok if you find it unsuitable but i kind of still want to popularize it.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1124
+                     (ReturnChildren &&
+                      ReturnChildren->getParent(SuspiciousStmt));
+            }
----------------



================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1132
+  /// Check if parameter with the given index was ever used.
+  /// NOTE: This function checks only for escapes.
+  bool wasEverUsed(unsigned Index) const {
----------------
So the contract of this function is "We haven't seen any actual calls, which is why we're wondering", otherwise it's not expected to do what it says it does?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1247
+                               State &ToAlter) const {
+    // Even when the analyzer is technically correct, it is a widespread pattern
+    // not to call completion handlers in some scenarios.  These usually have
----------------
(:


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1310
+  /// a specified status for the given parameter.
+  bool isAnyOfSuccessorsHasStatus(const CFGBlock *Parent,
+                                  unsigned ParameterIndex,
----------------
"Successor is has status" doesn't sound right, maybe `anySuccessorHasStatus`?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1442
+    // is intentionally not called on this path.
+    if (Cast->getType() == AC.getASTContext().VoidTy) {
+      checkEscapee(Cast->getSubExpr());
----------------
I feel really bad bringing this up in this context but i guess it kind of makes sense to canonicalize the type first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92039