[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
zturner added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC326113: Emit proper CodeView when -gcodeview is passed
without the cl driver. (authored by zturner, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D43700?vs=135723=135932#toc
Repository:
rC Clang
https://reviews.llvm.org/D43700
Files:
lib/Driver/ToolChains/Clang.cpp
test/Driver/codeview-column-info.c
Index: test/Driver/codeview-column-info.c
===
--- test/Driver/codeview-column-info.c
+++ test/Driver/codeview-column-info.c
@@ -0,0 +1,13 @@
+// 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"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2968,7 +2968,7 @@
// 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 &&
@@ -3567,6 +3567,8 @@
types::ID InputType = Input.getType();
if (D.IsCLMode())
AddClangCLArgs(Args, InputType, CmdArgs, , );
+ else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
const Arg *SplitDWARFArg = nullptr;
RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
Index: test/Driver/codeview-column-info.c
===
--- test/Driver/codeview-column-info.c
+++ test/Driver/codeview-column-info.c
@@ -0,0 +1,13 @@
+// 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"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2968,7 +2968,7 @@
// 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 &&
@@ -3567,6 +3567,8 @@
types::ID InputType = Input.getType();
if (D.IsCLMode())
AddClangCLArgs(Args, InputType, CmdArgs, , );
+ else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
const Arg *SplitDWARFArg = nullptr;
RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
zturner added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
smeenai added a comment. The `-g` meaning `-gcodeview` for MSVC environments change should be a separate diff though, right? https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
colden added a comment.
In https://reviews.llvm.org/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`).
https://reviews.llvm.org/D43700
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
zturner added a comment. @rnk by "in an MSVC environment" do you mean "when -fms-compatibility is present"? https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
colden added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
aganea added a comment. 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. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Lgtm We should go farther, IMO. `clang -g` should default to emitting codeview in an MSVC environment. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
smeenai added a comment. 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.) https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
probinson added a comment. 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! https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
colden added a comment. In https://reviews.llvm.org/D43700#1018087, @zturner wrote: > In https://reviews.llvm.org/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. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
zturner added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
colden added a comment. 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? https://reviews.llvm.org/D43700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43700: Emit proper CodeView even when not using the cl driver.
zturner created this revision.
zturner added a reviewer: rnk.
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.
https://reviews.llvm.org/D43700
Files:
clang/lib/Driver/ToolChains/Clang.cpp
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2965,7 +2965,7 @@
// 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 &&
@@ -3552,6 +3552,8 @@
types::ID InputType = Input.getType();
if (D.IsCLMode())
AddClangCLArgs(Args, InputType, CmdArgs, , );
+ else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
const Arg *SplitDWARFArg = nullptr;
RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2965,7 +2965,7 @@
// 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 &&
@@ -3552,6 +3552,8 @@
types::ID InputType = Input.getType();
if (D.IsCLMode())
AddClangCLArgs(Args, InputType, CmdArgs, , );
+ else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
const Arg *SplitDWARFArg = nullptr;
RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
