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

Fri Dec 4 18:32:57 PST 2020

dblaikie added inline comments.

================
Comment at: clang/test/Driver/debug-options.c:364-366
+// GEMBED_2:  warning: debug information option '-gembed-source' is not supported for target
 // NOGEMBED_5-NOT:  "-gembed-source"
+// NOGEMBED_2-NOT:  warning: debug information option '-gembed-source' is not supported for target
----------------
scott.linder wrote:
> tra wrote:
> > scott.linder wrote:
> > > dblaikie wrote:
> > > > This is a bit of a loss in fidelity - might need a new diagnostic message (or go hunting around for a more general purpose one than this one at least) to say '-gembed-source' is ignored when not using DWARFv5. (in the nvptx case it's "not supported on target", but in the existing cases covered by this test it's "not supported because the user requested DWARFv2", for instance)
> > > > 
> > > > @JDevlieghere & @scott.linder for thoughts on this.
> > > I agree that I'd prefer we detect whether the target-specific clamped version is to blame (and use the proposed warning message) or the original DWARF version is to blame (and use the old message).
> > > 
> > > If I were compiling for x86 and gave e.g. `-gdwarf-4 -gembed-source` and the error said "not supported by target" I'd probably get the wrong idea.
> > > 
> > > It would also be nice to retain the error semantics in the case where the user is explicitly requesting incompatible options.
> > This sounds pretty close to what the older iterations of this patch did:  https://reviews.llvm.org/D92617?id=309404, except that it preserved the current behavior which makes it an error to use -gembed-source with an explicitly specified DWARF version below 5. Same options with a lower clamped version produces a warning. I.e. If user specified a nominally valid combination of `-gdwarf-5 -gembed-source` but the target like NVPTX clamped it down to DWARF2, there will only be a warning.
> > 
> > I would appreciate if you (as in debug info stakeholders) could clarify couple of moments for me:
> > 
> > * should `-gdwarf-4 -gembed-source` be an error or warning? It's currently an error.
> > * `-gdwarf-5 -gembed-source` with the dwarf version clamped below 5 should produce a warning. `not supported for target` appears to be correct, but does not explain why. Do we want to amend/change it to say `because target only supports DWARF 2` or `target does not support DWARF 5`? Or is `not supported by target` sufficient as is?
> > 
> > 
> > This sounds pretty close to what the older iterations of this patch did:  https://reviews.llvm.org/D92617?id=309404, except that it preserved the current behavior which makes it an error to use -gembed-source with an explicitly specified DWARF version below 5. Same options with a lower clamped version produces a warning. I.e. If user specified a nominally valid combination of `-gdwarf-5 -gembed-source` but the target like NVPTX clamped it down to DWARF2, there will only be a warning.
> > 
> > I would appreciate if you (as in debug info stakeholders) could clarify couple of moments for me:
> > 
> > * should `-gdwarf-4 -gembed-source` be an error or warning? It's currently an error.
> 
> I think it should remain an error when an incompatible DWARF version is explicitly specified by the user. They said they wanted two things which are mutually exclusive, and we have no way to know which one they meant (or if they just aren't aware they are incompatible) so we should stop and prompt them to fix it.
> 
> > * `-gdwarf-5 -gembed-source` with the dwarf version clamped below 5 should produce a warning. `not supported for target` appears to be correct, but does not explain why. Do we want to amend/change it to say `because target only supports DWARF 2` or `target does not support DWARF 5`? Or is `not supported by target` sufficient as is?
> > 
> > 
> 
> I think giving more context is a good thing in this situation; I don't know that I have a strong opinion on the wording, but something indicating the warning is due to a target restriction which caps the DWARF version seems helpful enough to warrant being more verbose. I'd vote for your `because target only supports DWARF <N>`
> 
OK, sounds like we're going mostly back towards the originally proposed code.

Fair enough - if that's the case I'd make one suggestion to tweak the original code:

Rather than keeping DWARFVersion unclamped and having AdjustedDWARFVersion as well - It might work better to have DWARFVersion updated correctly (in the "if (EmitDwarf)" block) and save "UnadjustedDWARFVersion" (open to other naming suggestions) so that vaule can be consulted later to determine the appropriate error or warning to be emitted. But this way "DWARFVersion" does carry the to-be-emitted DWARF version which is more likely what the next developer to touch this code will be thinking about/wanting when they reach for that variable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92617