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. > > >> >> >>> >>> >>>> - 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
