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

Wed Apr 15 14:19:25 PDT 2020

gamesh411 marked 23 inline comments as done.
gamesh411 added a comment.

Answered most review comments. Thanks for the reviewers @balazske, @martong so far.
The question of absolute path policy is still up for debate.

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:365

+  llvm::SmallString<256> AbsPath = CTUDir;
+  llvm::sys::path::append(AbsPath, Identifier);
----------------
martong wrote:
> 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.
Actually this does not need to be absolute. The naming of the variable should be changed to PrefixedPath or similar. I will do that for now.

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:365

+  llvm::SmallString<256> AbsPath = CTUDir;
+  llvm::sys::path::append(AbsPath, Identifier);
----------------
gamesh411 wrote:
> martong wrote:
> > 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.
> Actually this does not need to be absolute. The naming of the variable should be changed to PrefixedPath or similar. I will do that for now.
It not necessarily an absolute path, only prefixed by CTUDir. There is no clear policy as to where to use strictly absolute paths and where to accept relative paths. IMHO this should be considered, and I am most certainly open for a discussion. In the meantime, I will just rename the variable to PrefixedPath.

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:413
+  Files.push_back(std::string(Identifier));
+  ClangTool Tool(*CompileCommands, Files, CI.getPCHContainerOperations());
+
----------------
martong wrote:
> 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`.
As for the automatic lifetime of Tool, I am inclined to say this is not the case. I have run my clang-build with sanitizers (undefined and memory), and no bad access or UB was reported. If someone has more domain knowledge about tooling and the lifetime issues that could arise, I would be thrilled to be enlightened.

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:413
+  Files.push_back(std::string(Identifier));
+  ClangTool Tool(*CompileCommands, Files, CI.getPCHContainerOperations());
+
----------------
gamesh411 wrote:
> martong wrote:
> > 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`.
> As for the automatic lifetime of Tool, I am inclined to say this is not the case. I have run my clang-build with sanitizers (undefined and memory), and no bad access or UB was reported. If someone has more domain knowledge about tooling and the lifetime issues that could arise, I would be thrilled to be enlightened.
As for the need to pass PCHContainerOperaitons:
I think you are right, and I will check if this modifies the behaviour of the import in any way. I am removing this for now.

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:443
+    /// be the absolute path of the file.
+    *SourceFilePath = Identifier.str();
+
----------------
martong wrote:
> 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
The policy of absolute and relative paths should be discussed. See my previous answer concerning AbsPath variable above. BTW `llvm::sys::path::is_absolute` is a solution for asserting this as I see it.

================
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.
----------------
martong wrote:
> Is this hunk related?
Seems unrelated, not sure how it got included. Fixing it.

================
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
----------------
martong wrote:
> 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.
I have done this by example. Some CodeGen lit tests use sed the same way I did here. Needed for Windows compatibility maybe? (note that %t could substitute into a path containing backslashes on Windows) However, on Windows platforms, the availability of sed is not guaranteed either. The backslashes are used as escape chars in the JSON, hence the need to double-escape them. No comments are provided on those test also... I will add something tho (and also in the cpp test).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665