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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:365
 
+  llvm::SmallString<256> AbsPath = CTUDir;
+  llvm::sys::path::append(AbsPath, Identifier);
----------------
Could we check somehow if `CTUDir` is indeed an absolute path? If not then probably we should bail out with an error. Though I am not sure if there is an easy and portable way in llvm:: to check for beeing an absolute path.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:375
+/// Load the AST from a source-file, which is supposed to be located inside the
+/// compilation database \p OnDemandParsingCommands. The compilation database
+/// can contain the path of the file under the key "file" as an absolute path,
----------------
There is no variable named as `OnDemandParsingCommands`, perhaps you made a rename but forgot to rename in the comments?


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:384
+/// the invocation inside ClangTool is always made with an absolute path. \p
+/// ASTSourcePath is assumed to be the lookup-name of the file, which comes from
+/// the Index. The Index is built by the \p clang-extdef-mapping tool, which is
----------------
There is no variable named as `ASTSourcePath`, perhaps you made a rename but forgot to rename in the comments?


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:413
+  Files.push_back(std::string(Identifier));
+  ClangTool Tool(*CompileCommands, Files, CI.getPCHContainerOperations());
+
----------------
martong wrote:
> martong wrote:
> > Seems like `Tool` has it's lifetime ended when `load()` finishes. But we have long living `ASTUnits` whose lifetime are longer then the `Tool` which created them. Is this not a problem?
> `CI.getPCHContainerOperations()` is probably not needed, because we are not going to exercise the ASTReader, so this could be a nullptr (default param value).
If `CI.getPCHContainerOperations()` is not needed then we can remove the `CI` member of `ASTOnDemandLoader`.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:443
+    /// be the absolute path of the file.
+    *SourceFilePath = Identifier.str();
+
----------------
Perhaps we should make sure that `Identifier` is indeed an absolute path and if not then we should emit an error. If a user do not use CodeChecker to generate the ExternalDefMapping it may contain relative paths. See e.g.: https://clang.llvm.org/docs/analyzer/user-docs/CrossTranslationUnit.html#manual-ctu-analysis


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:457
+    return llvm::make_error<IndexError>(
+        index_error_code::ambiguous_compile_commands_database);
+
----------------
Could we have a lit test for this case? I.e having a compile_commands.json with two ambiguous entries for the same file and then we should expect the compiler diag?
(Note, I am asking this because it is not immediate for me from the code that this will ever fire.)


================
Comment at: clang/test/Analysis/ctu-main.c:53
   clang_analyzer_eval(res == 6); // expected-warning{{TRUE}}
+  // Call something with uninitialized from the same function in which the implicit was called.
+  // This is necessary to reproduce a special bug in NoStoreFuncVisitor.
----------------
Is this hunk related?


================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:3
 // RUN: mkdir -p %t/ctudir2
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
-// RUN:   -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c
-// RUN: cp %S/Inputs/ctu-other.c.externalDefMap.txt %t/ctudir2/externalDefMap.txt
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze \
+// RUN: echo '[{"directory":"%S/Inputs","command":"clang -x c -std=c89 -c ctu-other.c","file":"ctu-other.c"}]' | sed -e 's/\\/\\\\/g' > %t/ctudir2/compile_commands.json
+// RUN: %clang_extdef_map %S/Inputs/ctu-other.c > %t/ctudir2/externalDefMap.txt
----------------
Why do we need `sed` here? I don't see any `\` in the compile commands json. Is it because of Windows builds? Could you please explain in a comment maybe in the previous line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665