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

Tue Apr 21 08:04:22 PDT 2020

whisperity added inline comments.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:21
+
+PCH based analysis
+__________________
----------------
I think it's PCH-based with a -.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:25
+These can be generated by the Clang Frontend itself, and must be arranged in a specific way in the filesystem.
+The index, which maps function USR names to PCH dumps containing them must also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly uses a compilation command database to
----------------
symbols' USR names

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:26
+The index, which maps function USR names to PCH dumps containing them must also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly uses a compilation command database to
+determine the compilation flags used.
----------------
martong wrote:
> Maybe just simply write `This tool uses a compilation command database ...`
> (Same below with the on-demand version.)
~~clang tool~~

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:26
+The index, which maps function USR names to PCH dumps containing them must also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly uses a compilation command database to
+determine the compilation flags used.
----------------
whisperity wrote:
> martong wrote:
> > Maybe just simply write `This tool uses a compilation command database ...`
> > (Same below with the on-demand version.)
> ~~clang tool~~
And perhaps add some cross-references to what a compilation database is, etc. These things are also documented within Clang's doc tree.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:31
+
+[PCH] Manual CTU Analysis
+-------------------------
----------------
Instead of a prefix, create an overview of PCH-based analysis, and add this and the next one as a 3rd-level heading?

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:111-114
+      -Xclang -analyzer-config -Xclang experimental-enable-naive-ctu-analysis=true \
+      -Xclang -analyzer-config -Xclang ctu-dir=. \
+      -Xclang -analyzer-config -Xclang ctu-on-demand-parsing=false \
+      -Xclang -analyzer-output=plist-multi-file \
----------------
Are these flags documented somewhere?

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:227-228
+compilation database in order to determine the exact compiler invocation used for each TU.
+The index, which maps function USR names to source files containing them must also be generated by the
+`clang-extdef-mapping` clang tool. The mapping of external definitions implicitly uses a compilation command database to
+determine the compilation flags used. Preferably the same compilation database should be used when generating the
----------------
Same here for comments from above.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:331-335
+  [INFO 2019-07-16 17:21] - To view results in the terminal use the "CodeChecker parse" command.
+  [INFO 2019-07-16 17:21] - To store results use the "CodeChecker store" command.
+  [INFO 2019-07-16 17:21] - See --help and the user guide for further options about parsing and storing the reports.
+  [INFO 2019-07-16 17:21] - ----=================----
+  [INFO 2019-07-16 17:21] - Analysis length: 0.659618854523 sec.
----------------
These lines could be removed, I think, they don't add anything to the presentation.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:336-337
+  [INFO 2019-07-16 17:21] - Analysis length: 0.659618854523 sec.
+  $ ls
+  compile_commands.json  foo.cpp main.cpp  reports
+  $ tree reports
----------------
Due to the lack of colours, perhaps you could use `ls -F` which suffixes each directory with a `/`: `reports/`.

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:387
+-------------------------------------------------------------
+We actively develop CTU with CodeChecker as a "runner" script, `scan-build-py` is not actively developed for CTU.
+`scan-build-py` has various errors and issues, expect it to work with the very basic projects only.
----------------
as ~~a runner script~~ the driver for this feature

================
Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:388
+We actively develop CTU with CodeChecker as a "runner" script, `scan-build-py` is not actively developed for CTU.
+`scan-build-py` has various errors and issues, expect it to work with the very basic projects only.
+
----------------
expect it to work only ...

(to emphasise the limitation)

================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:120
+    case index_error_code::ambiguous_compile_commands_database:
+      return "Compile commands database contains multiple references to the "
+             "same source file.";
----------------
martong wrote:
> xazax.hun wrote:
> > Unfortunately, this is a very common case for a large number of projects that supports multiple targets (e.g. 32 and 64 bits). Is there a plan to mitigate this problem?
> I don't think we could do that. We need to merge ASTs of those TUs that are linkable. Otherwise the merge will be unsuccessful because of certain mismatches in Decls (structural eq will fail).
> Even in CodeChecker we are planning to issue a hard error in these ambiguous cases, even with the PCH method too. (The error would be suppressible by the user of course, but if that is suppressed then we take no responsibility.)
More clever logging of link commands and creating separate CDBs for each target or each binary could help this cause, but that needs infrastructural capabilities from both the build system and the analysis driver (i.e. CodeChecker).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665