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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3250
   // -gline-directives-only supported only for the DWARF debug info.
   if (DWARFVersion == 0 && DebugInfoKind == codegenoptions::DebugDirectivesOnly)
     DebugInfoKind = codegenoptions::NoDebugInfo;
----------------
probinson wrote:
> After setting DWARFVersion above, when nothing was requested, this looks peculiar.  Is `DWARFVersion == 0` a proxy for `-gcodeview` ?
Yep, DWARFVersion shouldn't be set when none was requested - see discussion above between myself & @cmtice - that should leave this code fine/correct.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6174
+    DwarfVersion = ParseDebugDefaultVersion(getToolChain(), Args);
+
   if (DwarfVersion == 0)
----------------
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?


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1151
+      || Value > 5
+      || Value < 1)
+    TC.getDriver().Diag(diag::err_drv_invalid_int_value)
----------------
probinson wrote:
> Clang doesn't support DWARF v1.  I expect nobody does at this point (DWARF 2 came out in 1993).
You'd recommend/request moving this bound up to 2 (to be consistent with the fact that clang supports -gdwarf-2 technically (even though we probably don't produce fully conformant DWARFv2 these days, I would guess))?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69822/new/

https://reviews.llvm.org/D69822