[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

[David Blaikie via Phabricator via cfe-commits]

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