[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3957
+  RenderDebugEnablingArgs(Args, CmdArgs, DebugInfoKind,
+                          std::min(DWARFVersion, TC.getMaxDwarfVersion()),
                           DebuggerTuning);
----------------
tra wrote:
> dblaikie wrote:
> > I think, ideally, the DWARFVersion calculation would happen up `if (EmitDwarf) {` block where all default and explicit dwarf version calculations are done.
> > 
> > I guess it's not done that way because of the gembed-source error path? 
> That's part of it. `-gembed-source` does need to know both the specified DWARF version and the clamped one. I could move the calculation of the effective DWARF version back to where it was in the first version of the patch, and we'll need both the specified and the clamped values.
> That would bring back the need to explain which of the two DWARF versions to use, where, and why. In the current revision the impact on DWARF version clamping is localized to where it makes a difference. Neither is perfect, but the current version is a bit easier to explain, I think.
> 
> As for moving it under `if( EmitDwarf)`, the problem is that the DWARF version also affects cases when `EmitDwarf` is false, so the clamping needs to be done regardless. E.g. the `-gmlt` option that was used in our build break that uncovered the issue. We do not emit DWARF per se, but the `DefaultDWARFVersion` does affect the generation of the lineinfo debugging and we do need to clamp it.
> 
> 
Is the warning necessary? Could we let -gembed-source be an error if you're targeting NVPTX? (I realize that goes a bit against the idea of this patch, which is to keep DWARFv2 even when you've got a bunch of DWARFv5 flags that you presumably want for the rest of your builds)

Or, I guess another option, might be to make -gembed-source a warning for non-v5 DWARF in general, instead of an error, then the implementation could be generic.

What I'd really like is for the DWARFVersion to be computed as compactly as possible without the concept of multiple versions leaking out as much as possible.

> As for moving it under if( EmitDwarf), the problem is that the DWARF version also affects cases when EmitDwarf is false, so the clamping needs to be done regardless. E.g. the -gmlt option that was used in our build break that uncovered the issue.

That doesn't sound right to me, -gmlt qualifies as "emitting DWARF" and at least a quick/simple debug through the driver for "clang x.cpp -gmlt" does seem to lead to "EmitDwarf" being true and the block at line 3841 being reached. EmitDwarf is set to true in that case at 3834 because DebugInfoKind is not NoDebugInfo (it's DebugLineTablesOnly)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92617