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

Saleem Abdulrasool via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 11 17:50:58 PDT 2016 

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

More information about the cfe-commits mailing list