clang-cl: Add /showFilenames option (PR31957)

[hans]

Summary

Add a /showFilenames option for users who want clang to echo the currently compiled filename. MSVC does this echoing by default, and it's useful for showing progress in build systems that doesn't otherwise provide any progress report, such as MSBuild.

[steveire]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

I'll have to keep a patch around anyway for myself locally to remove this condition. But maybe someone else will benefit from this.

What do you think about changing CMake to so that this is passed by default when using Clang-CL?

[zturner]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

Do you mean llvms cmake? Or cmake itself? Which generator are you using?

[hans]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

Can't you just configure your build to pass the flag to clang-cl?

I suppose CMake could be made to do that by default when using clang-cl versions that support the flag and using the MSBuild generator, but that's for the CMake community to decide I think. Or perhaps our VS integration could do that, but it gets tricky because it needs to know whether the flag is supported or not.

[steveire]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

I mean upstream cmake. My suggestion is independent of the generator, but it would depend on what Brad decides anyway. I don't intend to implement it, just report it.

I only have one clang but I have multiple buildsystems, and I don't want to have to add a new flag too all of them. In each toplevel CMakeLists everyone will need this to make use of the flag:

# Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
# Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
# variable named "MSVC" and
# the way CMake variables and the "if()" command work requires this. 
if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
        AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
    # CMake does not have explicit Clang-CL support yet.
    # It should have a COMPILER_ID for it.
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
    # etc
endif()

Upstream CMake can have the logic to add the flag instead.

[thakis]

Looks good. If this is passed and we invoke lld-link, should we give that a similar flag and pass that to lld-link as well? I think link.exe also prints its outputs.

include/clang/Driver/CLCompatOptions.td

163 ↗

(On Diff #167893)

Can you add /showFilenames- (a no-op) too, for symmetry?

[zturner]

Canyou add a test that demonstrates that multiple input files on the command line will print all of them? Also does this really need to be a cc1 arg? Why not just print it in the driver?

[zturner]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

But then what if you don’t want it? There would be no way to turn it off. I don’t think it’s a good fit for cmake

792 ↗

(On Diff #167893)

Yes, and actually for this reason i was wondering if we can do it without a flag at all. What if we just set an magic environment variable? It handles Stephen’s use case (he can just set it globally), and MSBuild (we can set it in the extension).

[rnk]

I support continuing our practice of being silent by default, and not implementing anything like the noisy default version banner printing that MSVC does, or this filename printing. I think setting this flag is something for cmake or the VS integration to do. Only someone who knows something about the progress printing model can set it correctly.

lib/Driver/ToolChains/Clang.cpp

3533–3534 ↗

(On Diff #167893)

Is a -cc1 flag actually necessary? I was imagining we'd print it in the driver before we launch the compile job.

[steveire]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

But then what if you don’t want it? There would be no way to turn it off.

Oh, I thought that the last flag would dominate. ie, if cmake generated /showFilename and the user adds /showFilename-, then you would end up with

clang-cl.exe /showFilename /showFilename-

and it would not be shown. I guess that's not how it works.

Maybe users want to not show it, but not seeing the filename is a surprising first-time experience when porting from MSVC to clang-cl using Visual Studio.

However, I'll just drop out of the discussion and not make any bug to CMake. If anyone else feels strongly, they can do so.

Thanks!

[zturner]

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

Right, but if we update the VS Integration Extension so that MSBuild specifies the flag (or sets the environment variable, etc), then any time you use MSBuild (even if not through the IDE) you would get the output, so to the user it would look no different.

[hans]

In D52773#1252584, @zturner wrote:

Canyou add a test that demonstrates that multiple input files on the command line will print all of them?

Will do.

Also does this really need to be a cc1 arg? Why not just print it in the driver?

Yeah, I'll give printing from the driver a try.

include/clang/Driver/CLCompatOptions.td

163 ↗

(On Diff #167893)

Will do. Actually not just a no-op, but the last option should win.

lib/Driver/ToolChains/Clang.cpp

3533–3534 ↗

(On Diff #167893)

Yeah, that's probably better, but will take a bit more work to keep track of if and what to print in the compile job. I'll give it a try.

lib/Frontend/CompilerInvocation.cpp

792 ↗

(On Diff #167893)

I think we should avoid magic environment variables as much as possible.

[zturner]

I agree magic environment variables are bad, but without it we don’t
address the only current actual use we have for this, which is making the
vs integration print filenames. Detecting compiler version from inside the
integration is hard, but with an environment variable it’s very easy to
solve.

I’m not against a flag as the supported way, but I think we should also
have some backdoor into this functionality, because if we’re not going to
satisfy the only known use case, then maybe it’s better to not even do it
at all.

[thakis]

In D52773#1255093, @zturner wrote:

I agree magic environment variables are bad, but without it we don’t
address the only current actual use we have for this, which is making the
vs integration print filenames. Detecting compiler version from inside the
integration is hard

We need some solution to this anyhow; e.g. say "this now requires clang 8.0", or have a clang version dropdown in the UI (which defaults to the latest release), or something. We can't add an env var for every future flag that the vs integration might want to use.

[zturner]

In D52773#1255491, @thakis wrote:

In D52773#1255093, @zturner wrote:

I agree magic environment variables are bad, but without it we don’t
address the only current actual use we have for this, which is making the
vs integration print filenames. Detecting compiler version from inside the
integration is hard

We need some solution to this anyhow; e.g. say "this now requires clang 8.0", or have a clang version dropdown in the UI (which defaults to the latest release), or something. We can't add an env var for every future flag that the vs integration might want to use.

But it is very hard to automatically detect the version from the integration. I tried this and gave up because it was flaky. So sure, we can present a drop-down in the UI, but it could be mismatched, and that would end up creating more problems than it solves.

Right now we have exactly 1 use case for showing filenames from clang-cl, and there's no good way to satisfy that use case with a command line option.

I agree that we can't add an environment variable for every future flag that the VS integration might want to use, but until that happens, YAGNI. And if and when we do need it, if we manage to come up with a solution to the problem, we can delete support for the environment variable and make it use the new method.

[rnk]

It doesn't address the version skew issue, but my blanket argument against any new environment variable is the existance of things like _CL_ and CCC_OVERRIDE_OPTIONS. You can set any compiler flag you want from the environment, so we should standardize on one way of setting options: flags.

[hans]

Here's a version now using cc1 flags, but printing directly from the driver. Please take a look.

[rnk]

lgtm

lib/Driver/Job.cpp

319 ↗

(On Diff #168999)

Huh, a -cc1 action can have multiple inputs? Go figure.

test/Driver/cl-showfilenames.c

7–8 ↗

(On Diff #168999)

I think it'd be nice to have the test show that diagnostics come out interleaved between the filenames. You can use #pragma message in the input files or something else to create warnings.

[hans]

test/Driver/cl-showfilenames.c

7–8 ↗

(On Diff #168999)

Good idea, thanks.

[asukainori]

I'm sorry to disturb.

But I found this option is conflict with -flto. e.g.

clang-cl -fuse-ld=lld -flto=thin /showFilenames .\test.cpp

will output:

clang-cl: warning: argument unused during compilation: '/showFilenames' [-Wunused-command-line-argument]

cfe/trunk/include/clang/Driver/CLCompatOptions.td

def _SLASH_QUESTION : CLFlag<"?">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_Qvec : CLFlag<"Qvec">,
   HelpText<"Enable the loop vectorization passes">, Alias<fvectorize>;
 def _SLASH_Qvec_ : CLFlag<"Qvec-">,
   HelpText<"Disable the loop vectorization passes">, Alias<fno_vectorize>;
 def _SLASH_showIncludes : CLFlag<"showIncludes">,
   HelpText<"Print info about included files to stderr">,
   Alias<show_includes>;
+def _SLASH_showFilenames : CLFlag<"showFilenames">,
+  HelpText<"Print the name of each compiled file">;
+def _SLASH_showFilenames_ : CLFlag<"showFilenames-">,
+  HelpText<"Don't print the name of each compiled file (default)">;
 def _SLASH_source_charset : CLCompileJoined<"source-charset:">,
   HelpText<"Source encoding, supports only UTF-8">, Alias<finput_charset_EQ>;
 def _SLASH_execution_charset : CLCompileJoined<"execution-charset:">,
   HelpText<"Runtime encoding, supports only UTF-8">, Alias<fexec_charset_EQ>;
 def _SLASH_std : CLCompileJoined<"std:">,
   HelpText<"Language standard to compile for">;
 def _SLASH_U : CLJoinedOrSeparate<"U">, HelpText<"Undefine macro">,
   MetaVarName<"<macro>">, Alias<U>;

cfe/trunk/include/clang/Driver/Job.h

class Command {
 
   /// The list of program arguments (not including the implicit first
   /// argument, which will be the executable).
   llvm::opt::ArgStringList Arguments;
 
   /// The list of program arguments which are inputs.
   llvm::opt::ArgStringList InputFilenames;
 
+  /// Whether to print the input filenames when executing.
+  bool PrintInputFilenames = false;
+
   /// Response file name, if this command is set to use one, or nullptr
   /// otherwise
   const char *ResponseFile = nullptr;
 
   /// The input file list in case we need to emit a file list instead of a
   /// proper response file
   llvm::opt::ArgStringList InputFileList;
 
public:
   void setEnvironment(llvm::ArrayRef<const char *> NewEnvironment);
 
   const char *getExecutable() const { return Executable; }
 
   const llvm::opt::ArgStringList &getArguments() const { return Arguments; }
 
   /// Print a command argument, and optionally quote it.
   static void printArg(llvm::raw_ostream &OS, StringRef Arg, bool Quote);
+
+  /// Set whether to print the input filenames when executing.
+  void setPrintInputFilenames(bool P) { PrintInputFilenames = P; }
 };
 
 /// Like Command, but with a fallback which is executed in case
 /// the primary command crashes.
 class FallbackCommand : public Command {
 public:
   FallbackCommand(const Action &Source_, const Tool &Creator_,
                   const char *Executable_, const ArgStringList &Arguments_,

cfe/trunk/lib/Driver/Job.cpp

 void Command::setEnvironment(llvm::ArrayRef<const char *> NewEnvironment) {
   Environment.reserve(NewEnvironment.size() + 1);
   Environment.assign(NewEnvironment.begin(), NewEnvironment.end());
   Environment.push_back(nullptr);
 }
 
 int Command::Execute(ArrayRef<llvm::Optional<StringRef>> Redirects,
                      std::string *ErrMsg, bool *ExecutionFailed) const {
+  if (PrintInputFilenames) {
+    for (const char *Arg : InputFilenames)
+      llvm::outs() << llvm::sys::path::filename(Arg) << "\n";
+    llvm::outs().flush();
+  }
+
   SmallVector<const char*, 128> Argv;
 
   Optional<ArrayRef<StringRef>> Env;
   std::vector<StringRef> ArgvVectorStorage;
   if (!Environment.empty()) {
     assert(Environment.back() == nullptr &&
            "Environment vector should be null-terminated by now");
     ArgvVectorStorage = llvm::toStringRefArray(Environment.data());

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

if (Args.hasArg(options::OPT__SLASH_fallback) &&
     // In /fallback builds, run the main compilation even if the pch generation
     // fails, so that the main compilation's fallback to cl.exe runs.
     C.addCommand(llvm::make_unique<ForceSuccessCommand>(JA, *this, Exec,
                                                         CmdArgs, Inputs));
   } else {
     C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
   }
 
+  // Make the compile command echo its inputs for /showFilenames.
+  if (Output.getType() == types::TY_Object &&
+      Args.hasFlag(options::OPT__SLASH_showFilenames,
+                   options::OPT__SLASH_showFilenames_, false)) {
+    C.getJobs().getJobs().back()->setPrintInputFilenames(true);
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_pg))
     if (!shouldUseFramePointer(Args, Triple))
       D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
                                                       << A->getAsString(Args);
 
   // Claim some arguments which clang supports automatically.
 
   // -fpch-preprocess is used with gcc to add a special marker in the output to

cfe/trunk/test/Driver/cl-showfilenames.c

+// RUN: %clang_cl /c /showFilenames -- %s 2>&1 | FileCheck -check-prefix=show %s
+// RUN: %clang_cl /c /showFilenames -- %s %S/Inputs/wildcard*.c 2>&1 | FileCheck -check-prefix=multiple %s
+
+// RUN: %clang_cl /c -- %s 2>&1 | FileCheck -check-prefix=noshow %s
+// RUN: %clang_cl /c /showFilenames /showFilenames- -- %s 2>&1 | FileCheck -check-prefix=noshow %s
+
+
+#pragma message "Hello"
+
+// show: cl-showfilenames.c
+// show-NEXT: warning: Hello
+
+// multiple: cl-showfilenames.c
+// multiple-NEXT: warning: Hello
+// multiple: wildcard1.c
+// multiple-NEXT: wildcard2.c
+
+// noshow: warning: Hello
+// noshow-NOT: cl-showfilenames.c