r274991 - [clang-cl] Add support for /Zd

David Majnemer via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 11 12:29:58 PDT 2016


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.


>
>
>>
>>
>>>
>>>
>>>> - 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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160711/9113e9a2/attachment-0001.html>


More information about the cfe-commits mailing list