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

Hans Wennborg via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 28 18:03:31 PDT 2016 

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.

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