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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };
----------------
(The naming of this function feels a bit odd. `markAsNewDecl` or just `markNewDecl`?)


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413
+    unsigned, CTUMaxNodesMultiplier, "ctu-max-nodes-mul",
+    "We count the nodes for a normal single tu analysis. We multiply that "
+    "number with this config value then we divide the result by 100 to get "
----------------



================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413-419
+    "We count the nodes for a normal single tu analysis. We multiply that "
+    "number with this config value then we divide the result by 100 to get "
+    "the maximum number of nodes that the analyzer can generate while "
+    "exploring a top level function in CTU mode (for each exploded graph)."
+    "For example, 100 means that CTU will explore maximum as much nodes that "
+    "we explored during the single tu mode, 200 means it will explore twice as "
+    "much, 50 means it will explore maximum 50% more.", 100)
----------------
whisperity wrote:
> 
Couldn't this description here be simplified to say something along the lines of //"the percentage of single-TU analysed nodes that the CTU analysis is allowed to visit"//? Is the calculation method needed from the user's perspective? The examples talk about percentage too.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+
----------------
Is this configuration inherent to the static analyser, and not the //CrossTU// library?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+
----------------
whisperity wrote:
> Is this configuration inherent to the static analyser, and not the //CrossTU// library?
(Documentation for the options are missing.)


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:156
   llvm::PointerUnion<const Expr *, const Decl *> Origin;
+  mutable Optional<bool> Foreign; // From CTU.
 
----------------



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:814-816
+  /// Returns true if the CTU analysis is running its first phase.
+  /// Returns true in single TU (non-CTU) mode!
+  bool isCTUInFirtstPhase() { return Engine.getCTUWorkList(); }
----------------
How and why is this needed? Could you call it `isSingleTUOr1stPhaseCTU` instead?

Rather, if this is used as a distinction input in conditionals, could you invert the branches and have a function `isSecondPhaseCTU`, and do the inverted logic where this function is consumed?


================
Comment at: clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt:1
+11:c:@F at other# ctu-onego-other.cpp.ast
----------------
Why is there only 1 symbol in this file, when the file above contains two function definitions?


================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:22
 //
+// FIXME On-demand ctu should be tested in the same file that we have for the
+// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu)
----------------



================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.cpp:33
+
+// FIXME On-demand ctu should be tested in the same file that we have for the
+// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu)
----------------



================
Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:24
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+//
+// CallGraph: c->b
----------------
Are the lines below related to the execution of the command above? If not, could you please break the comment?


================
Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:28
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// b/c that happens in another TU.
+
----------------



================
Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:30
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus
----------------
One-go? And what does that refer to? Is "Onego" analysis the one this patch is introducing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773