On Fri, Jul 29, 2016 at 10:12 AM, Nico Weber <tha...@chromium.org> wrote: > 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?
I don't think we should invent cl-like spellings for gcc-style flags; exposing gcc-style flags in clang-cl is perfectly fine for those cases. What makes this one special is that /Zd did exist in msvc at some point, and apparently icl uses it too, so it's not really invented, but also not currently an msvc so it's somewhere in between. >> 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
