[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Please check the summary of the patch, it seems to contain old information as well.



================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:210-212
+Preferably the same compilation database should be used when generating the external definitions, and
+during analysis.  The analysis invocation must be provided with the directory which contains the mapping
+files, and the compilation database which is used to determine compiler flags.
----------------
This is obsolete information.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:234
+  public:
+    explicit ASTFileLoader(CompilerInstance &CI, StringRef CTUDir);
+
----------------
Is the `explicit` needed here?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:388-389
+                "parsing there is no need for pre-dumping ASTs. External "
+                "definition mapping is still needed, and a valid compilation "
+                "database with compile commands for the external TUs is also "
+                "necessary. Disabled by default.",
----------------
"valid compilation database" is this line still what we wish to say? A YAML is used, so most likely we need a "valid invocation list" or something.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:393-398
+ANALYZER_OPTION(
+    StringRef, CTUInvocationList, "ctu-invocation-list",
+    "The path to the YAML format file containing a mapping from source file "
+    "paths to command-line invocations represented as a list of arguments. "
+    "This invocation is used produce the source-file's AST.",
+    "invocations.yaml")
----------------
Maybe it would be worthwhile to add a dummy example like `main.cpp: g++ hello-world.cpp` which has the proper format into this help.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:23-24
 #include "clang/Index/USRGeneration.h"
-#include "llvm/ADT/Triple.h"
+#include "clang/Tooling/JSONCompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/Optional.h"
----------------
The idea here was to not depend on libtooling but these lines are still here!


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:34
 #include <fstream>
+#include <llvm/Option/ArgList.h>
 #include <sstream>
----------------
<> -> "" and order


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:122-123
       return "Load threshold reached";
+    case index_error_code::ambiguous_compilation_database:
+      return "Compilation database contains multiple references to the same "
+             "source file.";
----------------
still using terms "compilation database", is this intended?


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:583
+                 CommandLineArgs.begin(),
+                 [](auto &&CmdPart) { return CmdPart.c_str(); });
+
----------------
balazske wrote:
> Is here a special reason to use `auto &&` type (and not `auto &` or `std::string &`)? Probably this code is faulty: The `CmdPart` is moved out and then destructed, before the content of it (where the `c_str` points to) is used?
@balazske This is a generic lambda where `auto&&` is the universal reference. It is equivalent to writing `template <typename T> f(T&& param)`. It obeys the usual point of instantiation and reference cascading rules, if a `const std::string&` is given as `T`, `const std::string& &&` will become a single `&`.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:609
+  llvm::SourceMgr SM;
+  llvm::yaml::Stream InvocationFiles(*ContentBuffer, SM);
+
----------------
Are the headers from which these types come from included?


================
Comment at: clang/test/Analysis/Inputs/ctu-other.c:34-38
+// TODO: Support the GNU extension asm keyword as well.
+// Example using the GNU extension: asm("mov $42, %0" : "=r"(res));
 int inlineAsm() {
   int res;
+  __asm__("mov $42, %0"
----------------
Possibly unrelated change?


================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:1-15
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: cp "%s" "%t/ctu-on-demand-parsing.c"
+// RUN: cp "%S/Inputs/ctu-other.c" "%t/ctu-other.c"
+// Path substitutions on Windows platform could contain backslashes. These are escaped in the json file.
+// RUN: echo '[{"directory":"%t","command":"gcc -std=c89 -Wno-visibility ctu-other.c","file":"ctu-other.c"}]' | sed -e 's/\\/\\\\/g' > %t/compile_commands.json
+// RUN: echo '"%t/ctu-other.c": ["gcc", "-std=c89", "-Wno-visibility", "ctu-other.c"]' | sed -e 's/\\/\\\\/g' > %t/invocations.yaml
----------------
Does the LIT syntax allow for better organising these commands? This is now a bit hard to read


Something like this would help:
```
// RUN: rm -rf %t && mkdir -p %t
// RUN: cp "%s" "%t/ctu-on-demand-parsing.c" && cp "%S/Inputs/ctu-other.c" "%t/ctu-other.c"
// [empty]
// [comment]
// RUN: [json magic]
// RUN: [yaml magic]
// [empty]
// RUN: extdef map
// [empty]
// RUN: cc1
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665