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