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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
