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

Nico Weber via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 29 10:12:26 PDT 2016 

On Thu, Jul 28, 2016 at 9:03 PM, Hans Wennborg <hans at 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 at lists.llvm.org> wrote:
> > 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
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160729/2889f406/attachment-0001.html>

More information about the cfe-commits mailing list