[PATCH] D88715: [llvm-objdump] --source: drop the warning when there is no debug info

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1019-1021
+    reportWarning("failed to parse debug information for " + ObjectFilename +
+                      ": " + toString(ExpectedLineInfo.takeError()),
+                  ObjectFilename);
----------------
jhenderson wrote:
> MaskRay wrote:
> > rupprecht wrote:
> > > MaskRay wrote:
> > > > rupprecht wrote:
> > > > > `ObjectFilename` is in the warning twice now. It can be omitted from the error message string; passing it as the second arg should make it print something like: `warn: ObjectFilename: failed to parse debug information`
> > > > The code path is likely dead. This can be considered when ExpectedLineInfo does return an `Error`. (Note, there is no test for multiple warnings)
> > > I'm not sure what you mean by dead here -- are you saying `Symbolizer->symbolizeCode()` never returns an error?
> > `Symbolizer->symbolizeCode()` returning an `Error` is entirely untested in check-llvm.
> > 
> > By reading the code, I guess it may return an `Error` with a broken symbol table. But then, we would report a warning earlier in `disassembleObject`.
> > 
> > When `WarnedNoDebugInfo` was added, there was no test for multiple warning. It was a premature decision.
> > 
> > I can remove this piece entirely, but I think a TODO may be more appropriate.
> Isn't it possible for this code to be hit with broken debug info (e.g. a malformed line table)? I would be slightly surprised if code outside `printSourceLine` actually did the debug data parsing after all, whilst `symbolizeCode` will need to parse it after all.
Likely no (or it is a hidden code path I don't notice).

The default constructed DILineInfo is used to represent both (1) empty (2) invalid. The two states cannot be differentiated. D62462 calls them "<invalid>".


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1024-1030
-    if (!WarnedNoDebugInfo) {
-      std::string Warning =
-          "failed to parse debug information for " + ObjectFilename.str();
-      if (!ErrorMessage.empty())
-        Warning += ": " + ErrorMessage;
-      reportWarning(Warning, ObjectFilename);
-      WarnedNoDebugInfo = true;
----------------
jhenderson wrote:
> rupprecht wrote:
> > Dropping the `WarnedNoDebugInfo` bool means this will generate an arbitrary amount of warnings instead of just 1. I'm not sure if this is a good thing or not; on one hand, it'd be nice to know all the issues instead of tackling them one by one; on the other hand, it's not really objdump's job to report all the invalid bits of debug info; we should have a different tool for that (I'd rather run `llvm-dwarfdump --verify`)
> I personally would like one per unique message, similar to how llvm-readobj operates with `reportUniqueWarning`.
> 
> I don't find `llvm-dwarfdump --verify` to be very helpful - it's really for making sure the debug data makes complete sense against some somewhat arbitrary, and not always standards-compliant rules, rather than for flushing out parsing problems, which is what this was about.
When we have a warning, we can add the deduplicating back if we find a need for it. Currently the code path looks dead to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88715