scott.linder 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 ---------------- 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>` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92617/new/ https://reviews.llvm.org/D92617 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
