r274991 - [clang-cl] Add support for /Zd
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 29 10:20:47 PDT 2016
On Fri, Jul 29, 2016 at 10:12 AM, Nico Weber <thakis at chromium.org> wrote:
> On Thu, Jul 28, 2016 at 9:03 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> Sorry, I was supposed to chime in here.
>>
>> I don't have a strong opinion on this, but I don't think it's a
>> problem for us to allow the -gline-tables-only spelling in addition to
>> /Zd.
>>
>> It just doesn't seem like a big deal to me.
>
>
> Do you have any general guideline for how to spell flags we add to clang-cl?
> Here we ended up with adding both the regular clang spelling and an invented
> cl-like flag. Do you think that's generally what we should do? Or should we
> decide this on a case-by-case basis?
I don't think we should invent cl-like spellings for gcc-style flags;
exposing gcc-style flags in clang-cl is perfectly fine for those
cases.
What makes this one special is that /Zd did exist in msvc at some
point, and apparently icl uses it too, so it's not really invented,
but also not currently an msvc so it's somewhere in between.
>> On Mon, Jul 11, 2016 at 5:50 PM, Saleem Abdulrasool via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>> > On Mon, Jul 11, 2016 at 12:29 PM, David Majnemer via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >>
>> >>
>> >>
>> >> On Mon, Jul 11, 2016 at 12:18 PM, Nico Weber <thakis at chromium.org>
>> >> wrote:
>> >>>
>> >>> On Mon, Jul 11, 2016 at 12:19 PM, David Majnemer
>> >>> <david.majnemer at gmail.com> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Mon, Jul 11, 2016 at 9:03 AM, Nico Weber <thakis at chromium.org>
>> >>>> wrote:
>> >>>>>
>> >>>>> On Mon, Jul 11, 2016 at 11:51 AM, David Majnemer
>> >>>>> <david.majnemer at gmail.com> wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> On Mon, Jul 11, 2016 at 8:42 AM, Nico Weber <thakis at chromium.org>
>> >>>>>> wrote:
>> >>>>>>>
>> >>>>>>> On Mon, Jul 11, 2016 at 11:36 AM, David Majnemer
>> >>>>>>> <david.majnemer at gmail.com> wrote:
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> On Mon, Jul 11, 2016 at 7:18 AM, Nico Weber <thakis at chromium.org>
>> >>>>>>>> wrote:
>> >>>>>>>>>
>> >>>>>>>>> VS2013's cl.exe doesn't understand /Zd, 2015's doesn't either.
>> >>>>>>>>> This
>> >>>>>>>>> means people who want to ask clang-cl for line tables only will
>> >>>>>>>>> have to add
>> >>>>>>>>> this flag in some if(is_clang) block in their build file
>> >>>>>>>>> anyways. What's the
>> >>>>>>>>> advantage of giving this flag a spelling that's different from
>> >>>>>>>>> both cl and
>> >>>>>>>>> clang? With -gline-tables-only, an if(is_clang) works on Linux,
>> >>>>>>>>> Mac,
>> >>>>>>>>> Windows.
>> >>>>>>>>>
>> >>>>>>>>> (Even if there's a good case for /Zd, I don't think we should
>> >>>>>>>>> remove user-exposed flags without a strong reason, so even if we
>> >>>>>>>>> keep /Zd I
>> >>>>>>>>> think we should also keep exposing -gline-tables-only.)
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> Existing users of -gline-tables-only? I'd imagine any
>> >>>>>>>> responsible
>> >>>>>>>> users of -gline-tables-only would probably use their build system
>> >>>>>>>> to verify
>> >>>>>>>> that the flag exists. We have never released an official LLVM
>> >>>>>>>> which
>> >>>>>>>> supported it (LLVM 3.8 came out in early March and
>> >>>>>>>> -gline-tables-only was
>> >>>>>>>> exposed via clang-cl in mid March).
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> Ok, but why is /Zd better than -gline-tables-only?
>> >>>>>>
>> >>>>>>
>> >>>>>> I see a few reasons:
>> >>>>>> - It is less surprising for a debug flag in the cl world to be
>> >>>>>> called
>> >>>>>> /Zsomething instead of -gsomething.
>> >>>>>
>> >>>>>
>> >>>>> Eh, you'll have to look it up either way to find the flag.
>> >>>>
>> >>>>
>> >>>> I agree.
>> >>>>
>> >>>>>
>> >>>>> And when seeing the flag, the -g flag is more self-explanatory.
>> >>>>
>> >>>>
>> >>>> I disagree, I had no idea what that flag did until I started working
>> >>>> on
>> >>>> debug info.
>> >>>>
>> >>>>>
>> >>>>> Also, it is surprising that a clang-cl /-style flag doesn't work
>> >>>>> with
>> >>>>> cl, so it just moves the surprise around a bit.
>> >>>>
>> >>>>
>> >>>> It is surprising when that happens in either direction. If this is
>> >>>> considered a bug, we will never "fix" it.
>> >>>>
>> >>>>>
>> >>>>>
>> >>>>>>
>> >>>>>> - I think that avoiding invasion of their namespace, when possible,
>> >>>>>> is
>> >>>>>> a good thing. It makes it less likely that we will conflict with a
>> >>>>>> flag
>> >>>>>> they want to add in the future.
>> >>>>>
>> >>>>>
>> >>>>> I don't understand this point. Doesn't invading their namespace make
>> >>>>> collisions _more_ likely?
>> >>>>>
>> >>>>>>
>> >>>>>> - I imagine that if they wanted to add support back for
>> >>>>>> -gline-tables-only, they'd name it /Zd.
>> >>>>>
>> >>>>>
>> >>>>> Given they had this flag and removed it, they probably didn't like
>> >>>>> it
>> >>>>> very much :-)
>> >>>>>
>> >>>>>>
>> >>>>>> I'm sympathetic to the argument that "-gline-tables-only" is more
>> >>>>>> familiar to Linux/Mac OS X folks and that /Zd won't work across all
>> >>>>>> platforms.
>> >>>>>>
>> >>>>>> Here why I think that's ok:
>> >>>>>> - This is not really a new problem. If you want to select c++1z
>> >>>>>> you
>> >>>>>> get to spell it /std:c++latest for clang-cl and -std=c++14.
>> >>>>>
>> >>>>>
>> >>>>> Sure, but these are flags that clang/gcc and cl interpret. So if you
>> >>>>> have a gcc build and a visual studio build, it's fairly easy to get
>> >>>>> each
>> >>>>> going with clang / clang-cl.
>> >>>>>
>> >>>>> With /Zd, we're needlessly inventing a new spelling for an existing
>> >>>>> clang flag.
>> >>>>
>> >>>>
>> >>>> icl also supports /Zd.
>> >>>
>> >>>
>> >>> That's a big point in favor of this change, I think. Thanks for
>> >>> pointing
>> >>> it out.
>> >>>
>> >>>>>
>> >>>>> As far as I can see, the new name creates a (small) problem without
>> >>>>> solving one.
>> >>>>
>> >>>>
>> >>>> This is an attempt to reduce unneeded diversity.
>> >>>
>> >>>
>> >>> From my point of view, it increased diversity: Before, this feature
>> >>> had
>> >>> one name, now it has two, and the second name is our own invention.
>> >>> (But if
>> >>> icl also uses that flag, then it's less bad.)
>> >>
>> >>
>> >> It's not out invention, it's Microsoft's. icl (and now clang-cl) both
>> >> happen to still support it. If a build system wanted just line table
>> >> debug
>> >> info for a cl-like driver, they just need /Zd.
>> >>
>> >>>
>> >>>
>> >>>>>
>> >>>>> So far, when we wanted to add a flag that cl doesn't have and that
>> >>>>> clang has, we've always gone for the clang spelling of that flag.
>> >>>>> That seems
>> >>>>> like a good guideline to me.
>> >>>>
>> >>>>
>> >>>> Often but not always. For example, we have /Qvec instead of
>> >>>> -fvectorize
>> >>>> because icl has the flag.
>> >>>
>> >>>
>> >>> Hm, I'd (weakly) argue that that's not ideal either :-)
>> >>
>> >>
>> >> Why? MSVC has picked up flags from icl. See /Qvec-report.
>> >>
>> >>>
>> >>>
>> >>> Anyhow, I think this isn't a good change, and it sounds like you
>> >>> disagree
>> >>> with that. So I guess the next step here is that Hans gets to make a
>> >>> decision once he's back?
>> >>
>> >>
>> >> More opinions are always better.
>> >
>> >
>> > Well, in that case, my opinion, which arguably may be worthless, is to
>> > keep
>> > the /Zd spelling.
>> >
>> > /Zd is meant to only produce line number information (aka, line tables).
>> > Yes, this is different from the clang driver, but if you want to use GCC
>> > style arguments, why are you using the clang-cl frontend?
>> >
>> >>>
>> >>>>
>> >>>>
>> >>>>>
>> >>>>>
>> >>>>>>
>> >>>>>> - People who want harmony between Mac OS X, Linux and Windows on
>> >>>>>> the
>> >>>>>> driver side can just use clang++.
>> >>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> I'd consider it in poor form for users to take a hard dependency
>> >>>>>>>> on
>> >>>>>>>> a flag which only exists in a compiler which has never been
>> >>>>>>>> released.
>> >>>>>>>>
>> >>>>>>>> I'd agree with you if -gline-tables-only had made its way to an
>> >>>>>>>> actual release.
>> >>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> On Mon, Jul 11, 2016 at 10:08 AM, Nico Weber
>> >>>>>>>>> <thakis at chromium.org>
>> >>>>>>>>> wrote:
>> >>>>>>>>>>
>> >>>>>>>>>> This breaks existing users of -gline-tables-only. What's the
>> >>>>>>>>>> motivation for this change?
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>> On Sat, Jul 9, 2016 at 5:49 PM, David Majnemer via cfe-commits
>> >>>>>>>>>> <cfe-commits at lists.llvm.org> wrote:
>> >>>>>>>>>>>
>> >>>>>>>>>>> Author: majnemer
>> >>>>>>>>>>> Date: Sat Jul 9 16:49:16 2016
>> >>>>>>>>>>> New Revision: 274991
>> >>>>>>>>>>>
>> >>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=274991&view=rev
>> >>>>>>>>>>> Log:
>> >>>>>>>>>>> [clang-cl] Add support for /Zd
>> >>>>>>>>>>>
>> >>>>>>>>>>> MASM (ML.exe and ML64.exe) and older versions of MSVC (CL.exe)
>> >>>>>>>>>>> support a
>> >>>>>>>>>>> flag called /Zd which is more-or-less -gline-tables-only.
>> >>>>>>>>>>>
>> >>>>>>>>>>> It seems nicer to support this flag instead of exposing
>> >>>>>>>>>>> -gline-tables-only.
>> >>>>>>>>>>>
>> >>>>>>>>>>> Modified:
>> >>>>>>>>>>> cfe/trunk/include/clang/Driver/CLCompatOptions.td
>> >>>>>>>>>>> cfe/trunk/include/clang/Driver/Options.td
>> >>>>>>>>>>> cfe/trunk/lib/Driver/Tools.cpp
>> >>>>>>>>>>> cfe/trunk/test/Driver/cl-options.c
>> >>>>>>>>>>>
>> >>>>>>>>>>> Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td
>> >>>>>>>>>>> URL:
>> >>>>>>>>>>>
>> >>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=274991&r1=274990&r2=274991&view=diff
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> ==============================================================================
>> >>>>>>>>>>> --- cfe/trunk/include/clang/Driver/CLCompatOptions.td
>> >>>>>>>>>>> (original)
>> >>>>>>>>>>> +++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Sat Jul
>> >>>>>>>>>>> 9
>> >>>>>>>>>>> 16:49:16 2016
>> >>>>>>>>>>> @@ -166,6 +166,8 @@ def _SLASH_Zc_trigraphs_off : CLFlag<"Zc
>> >>>>>>>>>>> HelpText<"Disable trigraphs (default)">,
>> >>>>>>>>>>> Alias<fno_trigraphs>;
>> >>>>>>>>>>> def _SLASH_Z7 : CLFlag<"Z7">,
>> >>>>>>>>>>> HelpText<"Enable CodeView debug information in object
>> >>>>>>>>>>> files">;
>> >>>>>>>>>>> +def _SLASH_Zd : CLFlag<"Zd">,
>> >>>>>>>>>>> + HelpText<"Emit debug line number tables only">;
>> >>>>>>>>>>> def _SLASH_Zi : CLFlag<"Zi">, Alias<_SLASH_Z7>,
>> >>>>>>>>>>> HelpText<"Alias for /Z7. Does not produce PDBs.">;
>> >>>>>>>>>>> def _SLASH_Zp : CLJoined<"Zp">,
>> >>>>>>>>>>>
>> >>>>>>>>>>> Modified: cfe/trunk/include/clang/Driver/Options.td
>> >>>>>>>>>>> URL:
>> >>>>>>>>>>>
>> >>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=274991&r1=274990&r2=274991&view=diff
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> ==============================================================================
>> >>>>>>>>>>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>> >>>>>>>>>>> +++ cfe/trunk/include/clang/Driver/Options.td Sat Jul 9
>> >>>>>>>>>>> 16:49:16
>> >>>>>>>>>>> 2016
>> >>>>>>>>>>> @@ -1229,7 +1229,7 @@ def fdebug_prefix_map_EQ
>> >>>>>>>>>>> def g_Flag : Flag<["-"], "g">, Group<g_Group>,
>> >>>>>>>>>>> HelpText<"Generate source-level debug information">;
>> >>>>>>>>>>> def gline_tables_only : Flag<["-"], "gline-tables-only">,
>> >>>>>>>>>>> Group<gN_Group>,
>> >>>>>>>>>>> - Flags<[CoreOption]>, HelpText<"Emit debug line number
>> >>>>>>>>>>> tables
>> >>>>>>>>>>> only">;
>> >>>>>>>>>>> + HelpText<"Emit debug line number tables only">;
>> >>>>>>>>>>> def gmlt : Flag<["-"], "gmlt">, Alias<gline_tables_only>;
>> >>>>>>>>>>> def g0 : Flag<["-"], "g0">, Group<gN_Group>;
>> >>>>>>>>>>> def g1 : Flag<["-"], "g1">, Group<gN_Group>,
>> >>>>>>>>>>> Alias<gline_tables_only>;
>> >>>>>>>>>>>
>> >>>>>>>>>>> Modified: cfe/trunk/lib/Driver/Tools.cpp
>> >>>>>>>>>>> URL:
>> >>>>>>>>>>>
>> >>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=274991&r1=274990&r2=274991&view=diff
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> ==============================================================================
>> >>>>>>>>>>> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>> >>>>>>>>>>> +++ cfe/trunk/lib/Driver/Tools.cpp Sat Jul 9 16:49:16 2016
>> >>>>>>>>>>> @@ -6269,12 +6269,18 @@ void Clang::AddClangCLArgs(const
>> >>>>>>>>>>> ArgList
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> CmdArgs.push_back(Args.MakeArgString(Twine(LangOptions::SSPStrong)));
>> >>>>>>>>>>> }
>> >>>>>>>>>>>
>> >>>>>>>>>>> - // Emit CodeView if -Z7 is present.
>> >>>>>>>>>>> - *EmitCodeView = Args.hasArg(options::OPT__SLASH_Z7);
>> >>>>>>>>>>> - if (*EmitCodeView)
>> >>>>>>>>>>> - *DebugInfoKind = codegenoptions::LimitedDebugInfo;
>> >>>>>>>>>>> - if (*EmitCodeView)
>> >>>>>>>>>>> + // Emit CodeView if -Z7 or -Zd are present.
>> >>>>>>>>>>> + if (Arg *DebugInfoArg =
>> >>>>>>>>>>> + Args.getLastArg(options::OPT__SLASH_Z7,
>> >>>>>>>>>>> options::OPT__SLASH_Zd)) {
>> >>>>>>>>>>> + *EmitCodeView = true;
>> >>>>>>>>>>> + if
>> >>>>>>>>>>> (DebugInfoArg->getOption().matches(options::OPT__SLASH_Z7))
>> >>>>>>>>>>> + *DebugInfoKind = codegenoptions::LimitedDebugInfo;
>> >>>>>>>>>>> + else
>> >>>>>>>>>>> + *DebugInfoKind = codegenoptions::DebugLineTablesOnly;
>> >>>>>>>>>>> CmdArgs.push_back("-gcodeview");
>> >>>>>>>>>>> + } else {
>> >>>>>>>>>>> + *EmitCodeView = false;
>> >>>>>>>>>>> + }
>> >>>>>>>>>>>
>> >>>>>>>>>>> const Driver &D = getToolChain().getDriver();
>> >>>>>>>>>>> EHFlags EH = parseClangCLEHFlags(D, Args);
>> >>>>>>>>>>> @@ -9964,7 +9970,8 @@ void visualstudio::Linker::ConstructJob(
>> >>>>>>>>>>>
>> >>>>>>>>>>> CmdArgs.push_back("-nologo");
>> >>>>>>>>>>>
>> >>>>>>>>>>> - if (Args.hasArg(options::OPT_g_Group,
>> >>>>>>>>>>> options::OPT__SLASH_Z7))
>> >>>>>>>>>>> + if (Args.hasArg(options::OPT_g_Group,
>> >>>>>>>>>>> options::OPT__SLASH_Z7,
>> >>>>>>>>>>> + options::OPT__SLASH_Zd))
>> >>>>>>>>>>> CmdArgs.push_back("-debug");
>> >>>>>>>>>>>
>> >>>>>>>>>>> bool DLL = Args.hasArg(options::OPT__SLASH_LD,
>> >>>>>>>>>>> options::OPT__SLASH_LDd,
>> >>>>>>>>>>>
>> >>>>>>>>>>> Modified: cfe/trunk/test/Driver/cl-options.c
>> >>>>>>>>>>> URL:
>> >>>>>>>>>>>
>> >>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=274991&r1=274990&r2=274991&view=diff
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> ==============================================================================
>> >>>>>>>>>>> --- cfe/trunk/test/Driver/cl-options.c (original)
>> >>>>>>>>>>> +++ cfe/trunk/test/Driver/cl-options.c Sat Jul 9 16:49:16
>> >>>>>>>>>>> 2016
>> >>>>>>>>>>> @@ -420,7 +420,7 @@
>> >>>>>>>>>>> // Z7: "-gcodeview"
>> >>>>>>>>>>> // Z7: "-debug-info-kind=limited"
>> >>>>>>>>>>>
>> >>>>>>>>>>> -// RUN: %clang_cl -gline-tables-only /Z7 /c -### -- %s 2>&1 |
>> >>>>>>>>>>> FileCheck -check-prefix=Z7GMLT %s
>> >>>>>>>>>>> +// RUN: %clang_cl /Zd /c -### -- %s 2>&1 | FileCheck
>> >>>>>>>>>>> -check-prefix=Z7GMLT %s
>> >>>>>>>>>>> // Z7GMLT: "-gcodeview"
>> >>>>>>>>>>> // Z7GMLT: "-debug-info-kind=line-tables-only"
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> _______________________________________________
>> >>>>>>>>>>> cfe-commits mailing list
>> >>>>>>>>>>> cfe-commits at lists.llvm.org
>> >>>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >>
>> >> _______________________________________________
>> >> cfe-commits mailing list
>> >> cfe-commits at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>
>> >
>> >
>> >
>> > --
>> > Saleem Abdulrasool
>> > compnerd (at) compnerd (dot) org
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>
>
More information about the cfe-commits
mailing list