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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 15 inline comments as done.
martong added inline comments.


================
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };
----------------
whisperity wrote:
> (The naming of this function feels a bit odd. `markAsNewDecl` or just `markNewDecl`?)
Good point, `markAsNewDecl` sounds better. I'll update this in the parent patch https://reviews.llvm.org/D123685 though (and rebase this).


================
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:
> 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.
Thanks, this is so true, very good point. I've changed it. And also changed the name to suffix `-pct` instead of `-mul` to reflect this.


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


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+
----------------
martong wrote:
> whisperity wrote:
> > whisperity wrote:
> > > Is this configuration inherent to the static analyser, and not the //CrossTU// library?
> > (Documentation for the options are missing.)
> Yes, this is only for the analyzer.
This is the direct representation of the analyzer option `ctu-phase1-inlining` and there is a bulky documentation for that in `AnalyzerOptions.def`. I'd rather not repeat that documentation here.


================
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(); }
----------------
whisperity wrote:
> 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?
Yep, very good point, thanks again. Changed it with the inverted branch version.


================
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
----------------
whisperity wrote:
> Why is there only 1 symbol in this file, when the file above contains two function definitions?
Good catch. I've added the other function. Besides, I had the wrong filename provided, it should have been `ctu-onego-existingdef-other.cpp.ast`, the `existingdef` was missing. Fixed that too, plus added and extra `FileCheck` to the `existingdef` test that detects if the ast file is not loaded.


================
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
----------------
whisperity wrote:
> One-go? And what does that refer to? Is "Onego" analysis the one this patch is introducing?
Yes. I've updated the summary of the patch to make this clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773