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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> If sources are not available, it should be obvious as the output will have no interleaved source lines.

I'm not entirely convinced that this is a fair statement. I've had a number of occasions in the past where I've tried to dump the source for an object and then got confused why there was no source displayed. Often, it was because I'd forgotten to build with -g, but that wasn't always immediately obvious.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1019-1021
+    reportWarning("failed to parse debug information for " + ObjectFilename +
+                      ": " + toString(ExpectedLineInfo.takeError()),
+                  ObjectFilename);
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88715