Emit proper CodeView even when not using the cl driver.

[zturner]

Summary

Although the supported way of invoking clang on Windows is via the cl driver, some people may wish to use the g++ driver. and manually specify -gcodeview. This codepath is currently broken, as it will cause column information to be emitted, which causes debuggers to not work.

While we still don't claim to support clang on Windows without the cl driver, this is a straightforward patch to make things slightly better for people who want to go this route anyway.

[colden]

Seems good to me! I'll give it a test on my end.

One alternate implementation idea though, what if you defaulted EmitCodeView to the hasArg check instead of false, then removed the else *EmitCodeView = false; block on line 4999?

[zturner]

In D43700#1018042, @colden wrote:

Seems good to me! I'll give it a test on my end.

One alternate implementation idea though, what if you defaulted EmitCodeView to the hasArg check instead of false, then removed the else *EmitCodeView = false; block on line 4999?

That would actually change the behavior of the cl driver, which I kind of don't want to do since it's not necessary. It would whitelist an additional clang option to be recognized by the cl driver. Specifically, you could then get debug info via the cl driver without specifying /Z7 or /Zi. It makes the possibilities more confusing, and someone will invariably try to do it and screw something up. We already whitelist some dash options so that the cl driver will recognize them, but it's on a case by case basis and only when there's a strong need for it.

[colden]

In D43700#1018087, @zturner wrote:

In D43700#1018042, @colden wrote:

Seems good to me! I'll give it a test on my end.

One alternate implementation idea though, what if you defaulted EmitCodeView to the hasArg check instead of false, then removed the else *EmitCodeView = false; block on line 4999?

That would actually change the behavior of the cl driver, which I kind of don't want to do since it's not necessary. It would whitelist an additional clang option to be recognized by the cl driver. Specifically, you could then get debug info via the cl driver without specifying /Z7 or /Zi. It makes the possibilities more confusing, and someone will invariably try to do it and screw something up. We already whitelist some dash options so that the cl driver will recognize them, but it's on a case by case basis and only when there's a strong need for it.

Good point, that's a pretty good reason not to.

[probinson]

Being a cross-compiler I think it's generally a good thing to have more combinations be less broken.
Note that PS4's compiler is hosted on Windows and uses the gcc-style driver; it's convenient sometimes to be able to target Windows without having to learn a new driver language. So thanks for doing this!

[smeenai]

We were affected by this too. Thanks for fixing!

(We're actually using CodeView + DWARF, since we have a bunch of tooling built around DWARF. We use the gcc-style driver for legacy reasons.)

[rnk]

Lgtm

We should go farther, IMO. clang -g should default to emitting codeview in an MSVC environment.

[aganea]

Thanks for fixing this @zturner. I simply want to back-up what @probinson says above - most of the games we do at Ubisoft are currently using a different compilation toolchains for each platform (we ship at least 4-5 platforms for each top game). It can be clang or it can be MSVC; and most of the time it's different versions of clang. Our long-term goal is to preferably have only one toolchain for all our platforms & all our targets - that means one set of common g++ like flags, including Windows.

[zturner]

@rnk by "in an MSVC environment" do you mean "when -fms-compatibility is present"?

[colden]

In D43700#1019505, @aganea wrote:

Thanks for fixing this @zturner. I simply want to back-up what @probinson says above - most of the games we do at Ubisoft are currently using a different compilation toolchains for each platform (we ship at least 4-5 platforms for each top game). It can be clang or it can be MSVC; and most of the time it's different versions of clang. Our long-term goal is to preferably have only one toolchain for all our platforms & all our targets - that means one set of common g++ like flags, including Windows.

+1, this is what we're going for on Lumberyard as well. Well put.

[colden]

In D43700#1019506, @zturner wrote:

@rnk by "in an MSVC environment" do you mean "when -fms-compatibility is present"?

It looks like other places in this file are using Triple.isWindowsMSVCEnvironment(), which I think would make sense to use for this too. It's implemented as:

bool isWindowsMSVCEnvironment() const {
  return getOS() == Triple::Win32 &&
         (getEnvironment() == Triple::UnknownEnvironment ||
          getEnvironment() == Triple::MSVC);
}

This appears to default to true from a normal Windows cmd (on my Win10 machine, the default triple is x86_64-pc-windows-msvc).

[smeenai]

The -g meaning -gcodeview for MSVC environments change should be a separate diff though, right?

[zturner]

In D43700#1019533, @smeenai wrote:

The -g meaning -gcodeview for MSVC environments change should be a separate diff though, right?

Yes I'm not doing that in this patch. Just wanted to clarify what he meant. I'm writing a test for this right now and then I'll commit.

[zturner]

I had to revert this for now. It breaks the asan bots which expect column info. That fix is easy, but it also breaks a random linux bots which I don't have the ability to debug at the moment because my linux machine is not working. Hopefully I can get that resolved soon.

lib/Driver/ToolChains/Clang.cpp

static void RenderDebugOptions(const ToolChain &TC, const Driver &D,
   // If a -gdwarf argument appeared, remember it.
   if (const Arg *A =
           Args.getLastArg(options::OPT_gdwarf_2, options::OPT_gdwarf_3,
                           options::OPT_gdwarf_4, options::OPT_gdwarf_5))
     DWARFVersion = DwarfVersionNum(A->getSpelling());
 
   // Forward -gcodeview. EmitCodeView might have been set by CL-compatibility
   // argument parsing.
-  if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
+  if (EmitCodeView) {
     // DWARFVersion remains at 0 if no explicit choice was made.
     CmdArgs.push_back("-gcodeview");
   } else if (DWARFVersion == 0 &&
              DebugInfoKind != codegenoptions::NoDebugInfo) {
     DWARFVersion = TC.GetDefaultDwarfVersion();
   }
 
   // We ignore flag -gstrict-dwarf for now.
#endif
   // These two are potentially updated by AddClangCLArgs.
   codegenoptions::DebugInfoKind DebugInfoKind = codegenoptions::NoDebugInfo;
   bool EmitCodeView = false;
 
   // Add clang-cl arguments.
   types::ID InputType = Input.getType();
   if (D.IsCLMode())
     AddClangCLArgs(Args, InputType, CmdArgs, &DebugInfoKind, &EmitCodeView);
+  else
+    EmitCodeView = Args.hasArg(options::OPT_gcodeview);
 
   const Arg *SplitDWARFArg = nullptr;
   RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
                      IsWindowsMSVC, CmdArgs, DebugInfoKind, SplitDWARFArg);
 
   // Add the split debug info name to the command lines here so we
   // can propagate it to the backend.
   bool SplitDWARF = SplitDWARFArg && RawTriple.isOSLinux() &&

test/Driver/codeview-column-info.c

+// Check that -dwarf-column-info does not get added to the cc1 line:
+// 1) When -gcodeview is present via the clang or clang++ driver
+// 2) When /Z7 is present via the cl driver.
+
+// RUN: %clang -### -c -g -gcodeview %s 2> %t1
+// RUN: FileCheck < %t1 %s
+// RUN: %clangxx -### -c -g -gcodeview %s 2> %t2
+// RUN: FileCheck < %t2 %s
+// RUN: %clang_cl -### /c /Z7 %s 2> %t2
+// RUN: FileCheck < %t2 %s
+
+// CHECK: "-cc1"
+// CHECK-NOT: "-dwarf-column-info"