[clang][deps] NFC: Preserve the original frontend action

[jansvoboda11]

Summary

This patch stops adjusting the frontend action when clang-scan-deps is configured to use the full output format.

In a future patch, the dependency scanner needs to check whether the original compiler invocation builds a PCH. That's impossible when -Eonly et al. override -emit-pch.

The -Eonly flag is not needed - the dependency scanner explicitly sets up its own frontend action anyways.

[dexonsmith]

Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?

clang/tools/clang-scan-deps/ClangScanDeps.cpp

465–466

Nit: blank line before the comment might make it easier to read; also the second line seems to have an indent, is that intentional?

[jansvoboda11]

Fix whitespace around fixme

[jansvoboda11]

In D103461#2791760, @dexonsmith wrote:

Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?

The command-line adjustments are an implementation detail and there's a bunch of tests checking the make output format that exercise the code. So I'm not sure there's much value in explicitly testing the transformations. There's currently no way to print the adjusted command-line.

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

clang/tools/clang-scan-deps/ClangScanDeps.cpp

465–466

The indent is intentional, my IDE doesn't pick up unindented lines part of the FIXME. I've removed it so it's more in line with the rest of the codebase.

[dexonsmith]

Okay, LGTM. (Sorry for the delay, I've been out.)

In D103461#2793254, @jansvoboda11 wrote:

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

Please add that motivation to the FIXME itself.

[jansvoboda11]

In D103461#2815913, @dexonsmith wrote:

Okay, LGTM. (Sorry for the delay, I've been out.)

In D103461#2793254, @jansvoboda11 wrote:

My reason for the FIXME is that we could get rid of bunch of Windows-specific logic by adjusting CompilerInvocation instead.

Please add that motivation to the FIXME itself.

No problem, thanks for the review(s)!

[bmahjour]

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

[dexonsmith]

In D103461#2841918, @bmahjour wrote:

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

I think @jansvoboda11 is out this week (IIRC, back next week). A possible interim workaround would be to add an XFAIL: aix (or whatever the lit feature is for AIX triples)... are you able to land something like that to unblock you? (Seems like the correct fix for -fno-integrated-as could be a bit more complicated...)

[jansvoboda11]

In D103461#2841918, @bmahjour wrote:

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

Thanks for the report and the reproducer. I'll try to get a fix ready tomorrow.

[jansvoboda11]

In D103461#2862067, @jansvoboda11 wrote:

In D103461#2841918, @bmahjour wrote:

@jansvoboda11 This change is causing the following LIT tests to fail on AIX:

Clang :: ClangScanDeps/headerwithdirname.cpp
Clang :: ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

The reason seems to be related to the fact that -fno-integrated-as is on by default on that platform. I get the same failure on Linux if I change the "compilation database" file to add -fno-integrated-as to the "command" line:

> /build_llvm/bin/clang-scan-deps -compilation-database ./headerwithdirname.cpp.tmp.cdb -j 1
> Error while scanning dependencies for /build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/headerwithdirname_input.cpp:
error: unable to handle compilation, expected exactly one compiler job in ' "clang" "-cc1" "-triple" "powerpc64le-unknown-linux-gnu" "-S" ...;  "/usr/bin/as" "-a64" "-mppc64" "-mlittle-endian" "-mpower8" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir" "-I" "/build_llvm/tools/clang/test/ClangScanDeps/Output/headerwithdirname.cpp.tmp.dir/foodir" "-I" "Inputs" "-o" "headerwithdirname_input.o" "/tmp/headerwithdirname_input-2e0050.s"; '

Thanks for the report and the reproducer. I'll try to get a fix ready tomorrow.

Fixed here: https://reviews.llvm.org/D105695

clang/tools/clang-scan-deps/ClangScanDeps.cpp

int main(int argc, const char **argv) {
 
   llvm::cl::PrintOptionValues();
 
   // The command options are rewritten to run Clang in preprocessor only mode.
   auto AdjustingCompilations =
       std::make_unique<tooling::ArgumentsAdjustingCompilations>(
           std::move(Compilations));
   ResourceDirectoryCache ResourceDirCache;
+
+  // FIXME: Adjust the resulting CompilerInvocation in DependencyScanningAction
+  // instead of parsing and adjusting the raw command-line. This will make it
+  // possible to remove some code specific to clang-cl and Windows.
   AdjustingCompilations->appendArgumentsAdjuster(
       [&ResourceDirCache](const tooling::CommandLineArguments &Args,
                           StringRef FileName) {
         std::string LastO = "";
         bool HasMT = false;
         bool HasMQ = false;
         bool HasMD = false;
         bool HasResourceDir = false;
AdjustingCompilations->appendArgumentsAdjuster(
         // -o option.
         tooling::CommandLineArguments AdjustedArgs(Args.begin(), FlagsEnd);
         AdjustedArgs.push_back("-o");
 #ifdef _WIN32
         AdjustedArgs.push_back("nul");
 #else
         AdjustedArgs.push_back("/dev/null");
 #endif
-        if (!HasMT && !HasMQ) {
+        if (!HasMT && !HasMQ && Format == ScanningOutputFormat::Make) {
           // We're interested in source dependencies of an object file.
           std::string FileNameArg;
           if (!HasMD) {
             // FIXME: We are missing the directory unless the -o value is an
             // absolute path.
             FileNameArg = !LastO.empty() ? LastO : getObjFilePath(FileName);
           } else {
             FileNameArg = std::string(FileName);
           }
           if (ClangCLMode) {
             AdjustedArgs.push_back("/clang:-M");
             AdjustedArgs.push_back("/clang:-MT");
             AdjustedArgs.push_back(Twine("/clang:", FileNameArg).str());
           } else {
             AdjustedArgs.push_back("-M");
             AdjustedArgs.push_back("-MT");
             AdjustedArgs.push_back(std::move(FileNameArg));
           }
         }
         AdjustedArgs.push_back("-Xclang");
-        AdjustedArgs.push_back("-Eonly");
-        AdjustedArgs.push_back("-Xclang");
         AdjustedArgs.push_back("-sys-header-deps");
         AdjustedArgs.push_back("-Wno-error");
 
         if (!HasResourceDir) {
           StringRef ResourceDir =
               ResourceDirCache.findResourceDir(Args, ClangCLMode);
           if (!ResourceDir.empty()) {
             AdjustedArgs.push_back("-resource-dir");