[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via cfe-commits]

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.
On Thu, Oct 4, 2018 at 5:13 AM Hans Wennborg via Phabricator <
reviews at reviews.llvm.org> wrote:

> hans added a comment.
>
> In https://reviews.llvm.org/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.
>
>
>
> ================
> Comment at: include/clang/Driver/CLCompatOptions.td:163
> +  HelpText<"Print the name of each compiled file">,
> +  Alias<echo_main_file_name>;
>  def _SLASH_source_charset : CLCompileJoined<"source-charset:">,
> ----------------
> thakis wrote:
> > Can you add /showFilenames- (a no-op) too, for symmetry?
> Will do. Actually not just a no-op, but the last option should win.
>
>
> ================
> Comment at: lib/Driver/ToolChains/Clang.cpp:3533-3534
>    CmdArgs.push_back(getBaseInputName(Args, Input));
> +  if (Arg *A = Args.getLastArg(options::OPT_echo_main_file_name))
> +    A->render(Args, CmdArgs);
>
> ----------------
> rnk wrote:
> > Is a -cc1 flag actually necessary? I was imagining we'd print it in the
> driver before we launch the compile job.
> 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.
>
>
> ================
> Comment at: lib/Frontend/CompilerInvocation.cpp:792
>    Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
> +  if (Args.hasArg(OPT_echo_main_file_name))
> +    llvm::outs() << Opts.MainFileName << "\n";
> ----------------
> zturner wrote:
> > steveire wrote:
> > > zturner wrote:
> > > > zturner wrote:
> > > > > steveire wrote:
> > > > > > hans wrote:
> > > > > > > zturner wrote:
> > > > > > > > steveire wrote:
> > > > > > > > > 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?
> > > > > > > > >
> > > > > > > > Do you mean llvms cmake?  Or cmake itself?  Which generator
> are you using?
> > > > > > > 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.
> > > > > > 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.
> > > > > 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
> > > > 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).
> > > > 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!
> > 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.
> I think we should avoid magic environment variables as much as possible.
>
>
> https://reviews.llvm.org/D52773
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181004/4f140616/attachment.html>