[clang-cl] Print /showIncludes to stderr, if used in combination with /E, /EP or /P

[fxb]

Summary

This replicates 'cl.exe' behavior and allows for both preprocessor output and
dependency information to be extraced with a single compiler invocation.

This is especially useful for compiler caching with tools like Mozilla's sccache.

See: https://github.com/mozilla/sccache/issues/246

[fxb]

This is my first patch to clang, so any feedback regarding implementation appreciated!

Also, let me know if you have any suggestions on how to add more extensive tests for this.

[erichkeane]

include/clang/Frontend/DependencyOutputOptions.h

33–34

Doing these two as separate options is a touch annoying. First, there is an extra state that ends up being possible but ignored. I'd prefer making a "PrintShowIncludeDestination : 2" (name open for proper bikeshed) that contains an enum so that the states are explicit. Something like:
None,
StdOut,
StdErr

That way, the 4th state is explicitly unused.

test/Driver/cl-options.c

203

I'm perhaps missing something here... why did "/EP /P /showIncludes" previously NOT warn?

[fxb]

include/clang/Frontend/DependencyOutputOptions.h

33–34

Yes, that definitely makes sense and is a lot nicer than having these two options, which I found quite ugly as well.

I will fix that and upload a new patch set.

test/Driver/cl-options.c

203

/P will preprocess to a file, so it was compatible with /showIncludes, which was printed to stdout.

With this change all preprocessor options are now possible to use in combination with /showIncludes, since its output will be written to stderr, like cl.exe does.

I was actually not sure if I shall keep these tests at all after my change or just invert them, like I ended up doing here. There is probably a better way to actually test that /showIncludes output will end up on stderr, but with my limited experience I didn't figure that out yet. Let me know if you have any ideas how to improve these tests.

[fxb]

Updated the code to use an enum called ShowIncludesDestination

[erichkeane]

This looks fine to me. Let me know if you need me to commit this for you.

[fxb]

@erichkeane That would be great! Thanks!

[erichkeane]

When building this, it didn't build since you'd reused the name. I changed the variable to be named "Dest" instead of "Destination"

That said, the test suite now fails on Clang :: Frontend/print-header-includes.c'.

Please do a build with 'check-clang' to see why this no longer works.
Thanks!
-Erich

[fxb]

I'll have a look at these issues tomorrow and submit a new patch then.

Thanks for all your help so far!

[fxb]

1. Fixed the compile error caused by re-using the name ShowIncludesDestination. The member variable is now named ShowIncludesDest.
2. Fixed test/Frontend/print-header-includes.c to test both cases:
   • If only --show-includes is passed, includes are printed on stdout.
   • If both --show-includes and -E are passed, includes are printed on stderr.

include/clang/Frontend/DependencyOutputOptions.h

 #ifndef LLVM_CLANG_FRONTEND_DEPENDENCYOUTPUTOPTIONS_H
 #define LLVM_CLANG_FRONTEND_DEPENDENCYOUTPUTOPTIONS_H
 
 #include <string>
 #include <vector>
 
 namespace clang {
 
+/// ShowIncludesDestination - Destination for /showIncludes output.
+enum class ShowIncludesDestination { None, Stdout, Stderr };
+
 /// DependencyOutputFormat - Format for the compiler dependency file.
 enum class DependencyOutputFormat { Make, NMake };
 
 /// DependencyOutputOptions - Options for controlling the compiler dependency
 /// file generation.
 class DependencyOutputOptions {
 public:
   unsigned IncludeSystemHeaders : 1; ///< Include system header dependencies.
   unsigned ShowHeaderIncludes : 1;   ///< Show header inclusions (-H).
   unsigned UsePhonyTargets : 1;      ///< Include phony targets for each
                                      /// dependency, which can avoid some 'make'
                                      /// problems.
   unsigned AddMissingHeaderDeps : 1; ///< Add missing headers to dependency list
-  unsigned PrintShowIncludes : 1; ///< Print cl.exe style /showIncludes info.
   unsigned IncludeModuleFiles : 1; ///< Include module file dependencies.
 
+  /// Destination of cl.exe style /showIncludes info.
+  ShowIncludesDestination ShowIncludesDestination;
+
   /// The format for the dependency file.
   DependencyOutputFormat OutputFormat;
 
   /// The file to write dependency output to.
   std::string OutputFile;
 
   /// The file to write header include output to. This is orthogonal to
   /// ShowHeaderIncludes (-H) and will include headers mentioned in the
public:
   std::string ModuleDependencyOutputDir;
 
 public:
   DependencyOutputOptions() {
     IncludeSystemHeaders = 0;
     ShowHeaderIncludes = 0;
     UsePhonyTargets = 0;
     AddMissingHeaderDeps = 0;
-    PrintShowIncludes = 0;
     IncludeModuleFiles = 0;
+    ShowIncludesDestination = ShowIncludesDestination::None;
     OutputFormat = DependencyOutputFormat::Make;
   }
 };
 
 }  // end namespace clang
 
 #endif

lib/Driver/ToolChains/Clang.cpp

if (Args.hasArg(options::OPT__SLASH_Zl)) {
     CmdArgs.push_back(FlagForCRT.data());
 
     // This provides POSIX compatibility (maps 'open' to '_open'), which most
     // users want.  The /Za flag to cl.exe turns this off, but it's not
     // implemented in clang.
     CmdArgs.push_back("--dependent-lib=oldnames");
   }
 
-  // Both /showIncludes and /E (and /EP) write to stdout. Allowing both
-  // would produce interleaved output, so ignore /showIncludes in such cases.
-  if ((!Args.hasArg(options::OPT_E) && !Args.hasArg(options::OPT__SLASH_EP)) ||
-      (Args.hasArg(options::OPT__SLASH_P) &&
-       Args.hasArg(options::OPT__SLASH_EP) && !Args.hasArg(options::OPT_E)))
-    if (Arg *A = Args.getLastArg(options::OPT_show_includes))
-      A->render(Args, CmdArgs);
 
   // This controls whether or not we emit RTTI data for polymorphic types.
   if (Args.hasFlag(options::OPT__SLASH_GR_, options::OPT__SLASH_GR,
                    /*default=*/false))
     CmdArgs.push_back("-fno-rtti-data");
 
   // This controls whether or not we emit stack-protector instrumentation.
   // In MSVC, Buffer Security Check (/GS) is on by default.

lib/Frontend/CompilerInstance.cpp

if (!DepOpts.HeaderIncludeOutputFile.empty()) {
     StringRef OutputPath = DepOpts.HeaderIncludeOutputFile;
     if (OutputPath == "-")
       OutputPath = "";
     AttachHeaderIncludeGen(*PP, DepOpts,
                            /*ShowAllHeaders=*/true, OutputPath,
                            /*ShowDepth=*/false);
   }
 
-  if (DepOpts.PrintShowIncludes) {
+  if (DepOpts.ShowIncludesDestination != ShowIncludesDestination::None) {
     AttachHeaderIncludeGen(*PP, DepOpts,
                            /*ShowAllHeaders=*/true, /*OutputPath=*/"",
                            /*ShowDepth=*/true, /*MSStyle=*/true);
   }
 }
 
 std::string CompilerInstance::getSpecificModuleCachePath() {
   // Set up the module path, including the hash for the

lib/Frontend/CompilerInvocation.cpp

static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,
   Opts.OutputFile = Args.getLastArgValue(OPT_dependency_file);
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);
   Opts.IncludeModuleFiles = Args.hasArg(OPT_module_file_deps);
   Opts.UsePhonyTargets = Args.hasArg(OPT_MP);
   Opts.ShowHeaderIncludes = Args.hasArg(OPT_H);
   Opts.HeaderIncludeOutputFile = Args.getLastArgValue(OPT_header_include_file);
   Opts.AddMissingHeaderDeps = Args.hasArg(OPT_MG);
-  Opts.PrintShowIncludes = Args.hasArg(OPT_show_includes);
+  if (Args.hasArg(OPT_show_includes)) {
+    // Writing both /showIncludes and preprocessor output to stdout
+    // would produce interleaved output, so use stderr for /showIncludes.
+    // This behaves the same as cl.exe, when /E, /EP or /P are passed.
+    if (Args.hasArg(options::OPT_E) || Args.hasArg(options::OPT_P))
+      Opts.ShowIncludesDestination = ShowIncludesDestination::Stderr;
+    else
+      Opts.ShowIncludesDestination = ShowIncludesDestination::Stdout;
+  } else {
+    Opts.ShowIncludesDestination = ShowIncludesDestination::None;
+  }
   Opts.DOTOutputFile = Args.getLastArgValue(OPT_dependency_dot);
   Opts.ModuleDependencyOutputDir =
       Args.getLastArgValue(OPT_module_dependency_dir);
   if (Args.hasArg(OPT_MV))
     Opts.OutputFormat = DependencyOutputFormat::NMake;
   // Add sanitizer blacklists as extra dependencies.
   // They won't be discovered by the regular preprocessor, so
   // we let make / ninja to know about this implicit dependency.

lib/Frontend/HeaderIncludeGen.cpp

static void PrintHeaderInfo(raw_ostream *OutputFile, StringRef Filename,
   *OutputFile << Msg;
   OutputFile->flush();
 }
 
 void clang::AttachHeaderIncludeGen(Preprocessor &PP,
                                    const DependencyOutputOptions &DepOpts,
                                    bool ShowAllHeaders, StringRef OutputPath,
                                    bool ShowDepth, bool MSStyle) {
-  raw_ostream *OutputFile = MSStyle ? &llvm::outs() : &llvm::errs();
+  raw_ostream *OutputFile = &llvm::errs();
   bool OwnsOutputFile = false;
 
+  // Choose output stream, when printing in cl.exe /showIncludes style.
+  if (MSStyle) {
+    switch (DepOpts.ShowIncludesDestination) {
+    default:
+      llvm_unreachable("Invalid destination for /showIncludes output!");
+    case ShowIncludesDestination::Stderr:
+      OutputFile = &llvm::errs();
+      break;
+    case ShowIncludesDestination::Stdout:
+      OutputFile = &llvm::outs();
+      break;
+    }
+  }
+
   // Open the output file, if used.
   if (!OutputPath.empty()) {
     std::error_code EC;
     llvm::raw_fd_ostream *OS = new llvm::raw_fd_ostream(
         OutputPath.str(), EC, llvm::sys::fs::F_Append | llvm::sys::fs::F_Text);
     if (EC) {
       PP.getDiagnostics().Report(clang::diag::warn_fe_cc_print_header_failure)
           << EC.message();

test/Driver/cl-options.c

 // Qvec_-NOT: -vectorize-loops
 
 // RUN: %clang_cl /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes %s
 // showIncludes: --show-includes
 
 // RUN: %clang_cl /E /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E %s
 // RUN: %clang_cl /EP /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E %s
 // RUN: %clang_cl /E /EP /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E %s
-// showIncludes_E: warning: argument unused during compilation: '--show-includes'
-
-// RUN: %clang_cl /EP /P /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E_And_P %s
-// showIncludes_E_And_P-NOT: warning: argument unused during compilation: '--show-includes'
+// RUN: %clang_cl /EP /P /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E %s
+// showIncludes_E-NOT: warning: argument unused during compilation: '--show-includes'
 
 // /source-charset: should warn on everything except UTF-8.
 // RUN: %clang_cl /source-charset:utf-16 -### -- %s 2>&1 | FileCheck -check-prefix=source-charset-utf-16 %s
 // source-charset-utf-16: invalid value 'utf-16'
 
 // /execution-charset: should warn on everything except UTF-8.
 // RUN: %clang_cl /execution-charset:utf-16 -### -- %s 2>&1 | FileCheck -check-prefix=execution-charset-utf-16 %s
 // execution-charset-utf-16: invalid value 'utf-16'