On Thu, Jul 28, 2016 at 9:03 PM, Hans Wennborg <h...@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? > > On Mon, Jul 11, 2016 at 5:50 PM, Saleem Abdulrasool via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > On Mon, Jul 11, 2016 at 12:29 PM, David Majnemer via cfe-commits > > <cfe-commits@lists.llvm.org> wrote: > >> > >> > >> > >> On Mon, Jul 11, 2016 at 12:18 PM, Nico Weber <tha...@chromium.org> > wrote: > >>> > >>> On Mon, Jul 11, 2016 at 12:19 PM, David Majnemer > >>> <david.majne...@gmail.com> wrote: > >>>> > >>>> > >>>> > >>>> On Mon, Jul 11, 2016 at 9:03 AM, Nico Weber <tha...@chromium.org> > wrote: > >>>>> > >>>>> On Mon, Jul 11, 2016 at 11:51 AM, David Majnemer > >>>>> <david.majne...@gmail.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Mon, Jul 11, 2016 at 8:42 AM, Nico Weber <tha...@chromium.org> > >>>>>> wrote: > >>>>>>> > >>>>>>> On Mon, Jul 11, 2016 at 11:36 AM, David Majnemer > >>>>>>> <david.majne...@gmail.com> wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> On Mon, Jul 11, 2016 at 7:18 AM, Nico Weber <tha...@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 < > tha...@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@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@lists.llvm.org > >>>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >> > >> _______________________________________________ > >> cfe-commits mailing list > >> cfe-commits@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@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
