dblaikie added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:1955
Flags<[CC1Option]>;
+def fdebug_default_version: Joined<["-"], "fdebug-default-version=">,
Group<f_Group>;
def fdebug_prefix_map_EQ
----------------
probinson wrote:
> If this is specifically the default DWARF version, I think the word "dwarf"
> ought to be in the option name.
Can we haggle over this a bit?
My thinking behind -fdebug-default-version was consistency with other DWARF
related flags:
-fdebug-compilation-dir
-fdebug-info-for-profiling
-fdebug-macro
-fdebug-types-section
-fdebug-ranges-base-address
-fdebug-prefix-map
We do have some -fdwarf:
-fdwarf2-cfi-asm
-fdwarf-directory-asm
-fdwarf-exceptions
So I'm personally inclined to sticking with -fdebug* as being all DWARF
related/consistent there.
Thoughts?
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6174
+ DwarfVersion = ParseDebugDefaultVersion(getToolChain(), Args);
+
if (DwarfVersion == 0)
----------------
cmtice wrote:
> dblaikie wrote:
> > probinson wrote:
> > > I have to say, this sequence is a whole lot easier to follow than what's
> > > up in RenderDebugOptions. It would be nice if they were both so easy to
> > > understand.
> > >
> > > Although it makes me wonder, does `-fdebug-default-version` go unclaimed
> > > here if there's a `-gdwarf-2` on the command line?
> > Once the "else if (DefaultDWARFVersion != 0)" clause is removed, and the
> > ParseDebugDefaultVersion call is sunk down into the "if (EmitDwarf)" case -
> > hopefully that'll simplify the RenderDebugOptions code enough to be
> > reasonable?
> >
> > I think the complication there is that "-g" can imply CodeView if nothing
> > explicitly requests DWARF & the target is windows, whereas there's no
> > support for that kind of CV fallback here? So some of that might be
> > inherent.
> >
> > & agreed - worth testing that -fdebug-default-version doesn't get a
> > -Wunused warning if someone says "-fdebug-default-version 4 -gdwarf-2".
> > @cmtice is that tested? You might be able to add -Werror to the existing
> > test case's RUN lines to demonstrate that no such warnings are produced?
> The thought is that if the user actually chose a specific dwarf version, e.g.
> -gdwarf-2, then that value should override the general default, specified
> with -fdebug-default-version (soon to be
> -fdwarf-default-version).
That's the desired behavior, yes (-gdwarf-2 overrides this new flag) - what
@probinson is asking about is the behavior of -Wunused.
Clang's built-in flag handling does some work to warn on unused flags - it does
this based on which flags are queried by the API.
The risk is that, because ParseDwarfDefaultVersion is only called under the "if
(DwarfVersion == 0)" condition, which will be false if the user specified
-gdwarf-2, for instance, then the ParseDwarfDefaultVersion won't be called &
clang's unused flag handling may see the new flag as "unused" and warn about it.
Basically the question is (could you test this manually & confirm the behavior,
and check that the automated tests cover this - which I think they can cover
this with minimal effort by adding -Werror to some RUN lines in the tests),
does this produce a warning:
clang -gdwarf-2 -fdebug-default-version=5 x.s
(& if it doesn't warn, it might be worth understanding why it doesn't, I guess,
given the above hypothesis)
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3231
+ unsigned DWARFVersion = 0;
+ unsigned DefaultDWARFVersion = ParseDwarfDefaultVersion(TC, Args);
if (EmitDwarf) {
----------------
Can you move this down to the narrower scope:
if (unsigned DefaultDWARFVersion = ParseDefaultVersion(TC, Args))
DWARFVersion = DefaultDWARFVersion;
(but maybe that runs into the -Wunused warning issues discussed in the other
thread in this review)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69822/new/
https://reviews.llvm.org/D69822
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
