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

Wed Feb 3 00:22:33 PST 2021

jhenderson added a comment.

One small comment, then LGTM.

In D88715#2537636 <https://reviews.llvm.org/D88715#2537636>, @MaskRay wrote:

> As of whether it is useful to only report the warning when the executable has debug info but one object file does not have debug info: I think the proposal is still controversial. 
> (objdump -S does not have such a behavior and it could be argued that the behavior needs to be under an explicit option instead of being automatic).

I think that's the opposite of what I proposed? I suggested warning if all the objects were missing debug info (which as you mentioned earlier boils down to an existance check for `.debug_line` or similar). I agree that warning if some objects are missing debug_info is impractical, since as you point out, most executables will include CRT files which don't contain debug data, if nothing else.

> So I'll suggest we restore to the previous state where we don't warn for missing debug info. Note: we don't warn for malformed debug info. That hasn't been properly fixed by D62462 <https://reviews.llvm.org/D62462>.

Perhaps we could reopen the original PR and suggest a better fix or two (i.e. add something to achieve the above/properly report invalid debug data). WDYT?

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1024-1025
+    // TODO Untested.
+    reportWarning("failed to parse debug information for " + ObjectFilename +
+                      ": " + toString(ExpectedLineInfo.takeError()),
+                  ObjectFilename);
----------------
I think @rupprecht mentioned this already somewhere in one of these comments - in the event this warning does ever get triggered, the warning will be something like: `warning: 'foo.o': failed to parse debug information for foo.o: ...`. We don't need `ObjectFilename` twice, I think - just dropping `+ ObjectFilename +` should be enough.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88715