[PATCH] D64297: [JSONCompilationDatabase] Strip distcc/ccache/gomacc wrappers from parsed commands.

[Sam McCall via Phabricator via llvm-commits]

sammccall marked 4 inline comments as done.
sammccall added a comment.

In D64297#1572872 <https://reviews.llvm.org/D64297#1572872>, @MaskRay wrote:

> Yes. It also lacks user settings. Downstream `compile_commands.json` consumers may provide their own command line option filtering mechanism that users can customize and add more compiler schedulers. For some tools, they probably don't want to see filtering applying on multiple layers, especially if the filtering mechanism done at the JSONCompilationDatabase layer may have false positives.


I've been careful here to be conservative, I'd be interested if there's potential for false positives.

Regarding redundant filtering: it's a plausible concern in the abstract.
None of the in-tree tools have layers accounting for these wrappers, though. And I don't think we really want only some tools handling obscure build configurations, so this seems to be the right layer.
Generally speaking, it's up to out-of-tree tools to avoid duplication with upstream APIs, rather than the other way around.

>> but a messy aspect of the nature of the ecosystem around compile_commands.json.
> 
> I'll appreciate it if you propose something to make `cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On` `ninja -t comdb` etc more precise:)

The gn/cmake features Petr described sound good to me. For ninja, given its abstraction level I think solving it there is actually worse than solving it here.



================
Comment at: clang/lib/Tooling/JSONCompilationDatabase.cpp:272
+    bool HasCompiler =
+        (Args[1][0] != '-') && !llvm::sys::path::has_extension(Args[1]);
+    if (HasCompiler) {
----------------
MaskRay wrote:
> If you do this, it may not work with `ccache gcc.exe`. If `ccache/gomacc/distcc` cannot be used as the compiler driver, just remove the check of `HasCompiler`?
> If you do this, it may not work with ccache gcc.exe

Right! Fixed this so both `ccache` and `gcc` can have an exe extension.

> If ccache/gomacc/distcc cannot be used as the compiler driver, just remove the check of HasCompiler?

They can. There are three modes:
 - `ccache gcc file.c` (the mode we're trying to match)
 - `ccache file.c` (the mode we're trying to avoid matching)
 - `g++ file.c` (with g++ symlinked to ccache - we don't even notice)

Rewrote the comment to explain these cases more clearly.


================
Comment at: clang/lib/Tooling/JSONCompilationDatabase.cpp:292
+      Arguments.push_back(Node->getValue(Storage));
+  while (unwrapCommand(Arguments))
+    ;
----------------
MaskRay wrote:
> This loop may be too much. One-shot unwrapCommand(Arguments) may just work
Using `ccache` and `distcc` together is a common pattern.

(Is there a specific problem? Obviously it is possible to leave this out, but I can't see how it could cause problems that wouldn't occur with a single wrapper)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64297