[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
 
+  const bool Foreign = false; // From CTU.
+
----------------
I feel that we use different terms for the imported declarations. Sometimes we call them `new`, sometimes `imported`, sometimes `foreign`. In case all of these means the same thing, it would be nice to standardize on a single way of naming. If there is a subtle difference between them, let's document that in a comment. It would be nice if we did not need the comment after the declaration but it would be obvious from the variable name.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:154
   llvm::PointerUnion<const Expr *, const Decl *> Origin;
+  mutable Optional<bool> Foreign; // From CTU.
 
----------------
Why do we need this to be mutable? Shouldn't we have this information when the CallEvent is created?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:176
   WorkList *getWorkList() const { return WList.get(); }
+  WorkList *getCTUWorkList() const { return CTUWList.get(); }
 
----------------
I think we do not expect the STU WorkList to ever be null, maybe it is time to clean this up and return a reference. 


================
Comment at: clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:91
+               .Default(None);
+  assert(K.hasValue() && "CTU inlining mode is invalid.");
+  return K.getValue();
----------------
The `CTUPhase1InliningMode` is coming from the user, right? Unless we have some validation in place before this code get called, I think it might not be a good idea to assert fail on certain user inputs.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:432
+
+bool ExprEngine::ctuBifurcate(const CallEvent &Call, const Decl *D,
+                              NodeBuilder &Bldr, ExplodedNode *Pred,
----------------
What is the meaning if the return value? It looks like the function always returns true.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+    }
+    const bool BState = State->contains<CTUDispatchBifurcationSet>(D);
+    if (!BState) { // This is the first time we see this foreign function.
----------------
So if we see the same foreign function called in multiple contexts, we will only queue one of the contexts for the CTU. Is this the intended design? 

So if I see:
```
foreign(true);
foreign(false);
```

The new CTU will only evaluate `foreign(true)` but not `foreign(false)`. 


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:516
+  // function in the main TU.
+  if (Engine.getCTUWorkList())
+    // Mark the decl as visited.
----------------
So `getCTUWorkList` will return `nullptr` in the second phase? Reading this code first I had the impression we will skip doing marking them visited in the first phase. I think having a helper like `IsSecondCTUPhase` or something would make this a bit less confusing.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:1130
       // We are not bifurcating and we do have a Decl, so just inline.
-      if (inlineCall(*Call, D, Bldr, Pred, State))
+      if (ctuBifurcate(*Call, D, Bldr, Pred, State))
         return;
----------------
I think this is getting really confusing here. The comment saying we are not bifurcating and we call `ctuBifurcate`. While I do understand these are two different kinds of bifurcation but I think we should rethink how to name some of these functions. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773