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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits