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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3957
+  RenderDebugEnablingArgs(Args, CmdArgs, DebugInfoKind,
+                          std::min(DWARFVersion, TC.getMaxDwarfVersion()),
                           DebuggerTuning);
----------------
dblaikie wrote:
> 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)
> 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)

I think the warning is the sensible thing to do here. Making it an error would effectively make it impossible to use it on the host side of the compilation where it's perfectly fine and where it's most useful.
At the same time, we already issue a warning for some debugging options if a back-end does not support them, so a warning for `-gembed-source` follows the established pattern, even if it's handled as a special case.

> 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.

That would work. Then we'll only need the capped dwarf version and that could be moved upward. I've updated the patch. PTAL.

> that doesn't sound right to me, -gmlt qualifies as "emitting DWARF" 

You are correct. Even when we only emit debug info via `.loc`, EmitDwarf is set.







Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92617