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

[MaskRay]

Summary

Warnings have been added for three cases (PR41905): (1) missing debug info, (2)
the source file cannot be found, (3) the debug info points at a line beyond the
end of the file.

(1) is probably less useful. This was brought up once on
http://lists.llvm.org/pipermail/llvm-dev/2020-April/141264.html and two
internal users mentioned it to me that it was annoying. (I personally
find the warning confusing, too.)

Users specify --source to get additional information if sources happen to be
available. If sources are not available, it should be obvious as the output
will have no interleaved source lines. The warning can be especially annoying
when using llvm-objdump -S on a bunch of files.

This patch drops the warning when there is no debug info.
(If LLVMSymbolizer::symbolizeCode returns an Error, there will still be
an error. There is currently no test for an Error return value.
The only code path is probably a broken symbol table, but we probably already emit a warning
in that case)

source-interleave-prefix.test has an inappropriate "malformed" test - the test simply has no
.debug_* because new llc does not produce debug info when the filename is empty (invalid).
I have tried tampering the header of .debug_info/.debug_line but llvm-symbolizer does not warn.
This patch does not intend to add the missing test coverage.

[MaskRay]

Use Twine

[rupprecht]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1026

It would be good to figure out a test case for this

1027–1029

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

1029–1035

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)

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1026

The last paragraph of the description mentions this.

"The only code path is probably a broken symbol table, but we probably already emit a warning
in that case" (I have checked various code paths)

If it is a symbol table problem, it should not be a "debug info problem".

1027–1029

The code path is likely dead. This can be considered when ExpectedLineInfo does return an Error. (Note, there is no test for multiple warnings)

[rupprecht]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1026

The error message still says "failed to parse debug information", did you have plans to change that if it's not a debug info problem?

1027–1029

I'm not sure what you mean by dead here -- are you saying Symbolizer->symbolizeCode() never returns an error?

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1026

It says "failed to parse debug information". The code path is likely dead now. If future changes to Symbolizer->symbolizeCode makes it actually possible to return an Error, this will give some diagnostics.

This diagnostic was added to report "no debug info", which isn't an appropriate message. But I don't want to remove it entirely either.

1027–1029

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.

[jhenderson]

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.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1027–1029

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.

1029–1035

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.

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1027–1029

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>".

1029–1035

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.

[MaskRay]

In D88715#2313976, @jhenderson wrote:

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.

Seems like we may have disagreement. Let me rephrase things:

(1) When -g is not used, do you expect a warning "failed to parse debug info"?

This looks very confusing to me (D62462). I want to get rid of it despite some value losing for (2), and hence this patch.

(2) When -g is not used for the object, do you expect a warning?

I am a bit on the fence. If for places where you *could* have had source if only the right command line options (-g) were used, a warning might be useful.

There are also situations where a binary has some hand-written assembly components which normally do not attached debug info. If the noise ratio is low, I think I will be fine if there is a warning for each such component.
I will be on board if @mmpozulp or @jhenderson can enhance this in a non-confusing way.
The warning requires a more sophisticated approach, like checking whether there are bytes not covered by address ranges and/or line tables, not something as simple as what D62462 did: "if DILineInfo is invalid (invalid can mean either invalid or empty), emit a warning".

[jhenderson]

In D88715#2322240, @MaskRay wrote:

In D88715#2313976, @jhenderson wrote:

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.

Seems like we may have disagreement. Let me rephrase things:

(1) When -g is not used, do you expect a warning "failed to parse debug info"?

This looks very confusing to me (D62462). I want to get rid of it despite some value losing for (2), and hence this patch.

(2) When -g is not used for the object, do you expect a warning?

I am a bit on the fence. If for places where you *could* have had source if only the right command line options (-g) were used, a warning might be useful.

There are also situations where a binary has some hand-written assembly components which normally do not attached debug info. If the noise ratio is low, I think I will be fine if there is a warning for each such component.
I will be on board if @mmpozulp or @jhenderson can enhance this in a non-confusing way.
The warning requires a more sophisticated approach, like checking whether there are bytes not covered by address ranges and/or line tables, not something as simple as what D62462 did: "if DILineInfo is invalid (invalid can mean either invalid or empty), emit a warning".

My personal thoughts are that if there is any debug data in the binary, then I don't think we should warn, even if it means there's bits of the binary that aren't covered by any debug data - this should keep the logic nice and simple and helps avoid spurious warnings where e.g. the linker has added padding between functions. Thus, a linked object with at least one object with debug data will report no warning, whilst a linked object with no debug data at all, or an unlinked object without debug data would report "unable to find debug data" (or something like that). Does this make sense? I don't feel that strongly about it, so if this sounds controversial, I'm happy to hear other suggestions.

[MaskRay]

In D88715#2324359, @jhenderson wrote:

In D88715#2322240, @MaskRay wrote:

In D88715#2313976, @jhenderson wrote:

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.

Seems like we may have disagreement. Let me rephrase things:

(1) When -g is not used, do you expect a warning "failed to parse debug info"?

This looks very confusing to me (D62462). I want to get rid of it despite some value losing for (2), and hence this patch.

(2) When -g is not used for the object, do you expect a warning?

I am a bit on the fence. If for places where you *could* have had source if only the right command line options (-g) were used, a warning might be useful.

There are also situations where a binary has some hand-written assembly components which normally do not attached debug info. If the noise ratio is low, I think I will be fine if there is a warning for each such component.
I will be on board if @mmpozulp or @jhenderson can enhance this in a non-confusing way.
The warning requires a more sophisticated approach, like checking whether there are bytes not covered by address ranges and/or line tables, not something as simple as what D62462 did: "if DILineInfo is invalid (invalid can mean either invalid or empty), emit a warning".

My personal thoughts are that if there is any debug data in the binary, then I don't think we should warn, even if it means there's bits of the binary that aren't covered by any debug data - this should keep the logic nice and simple and helps avoid spurious warnings where e.g. the linker has added padding between functions. Thus, a linked object with at least one object with debug data will report no warning, whilst a linked object with no debug data at all, or an unlinked object without debug data would report "unable to find debug data" (or something like that). Does this make sense? I don't feel that strongly about it, so if this sounds controversial, I'm happy to hear other suggestions.

The internal user who reported the problem is fine with the approach (if any object has debug info, suppress the warning). This probably makes real world applications less annoying since (crt1.o for exe) crtbegin.o crtend.o are linked into the binary and they typically don't have debug info. Then this appears to boil down to an existence check of .debug_line.

[jhenderson]

In D88715#2325275, @MaskRay wrote:

The internal user who reported the problem is fine with the approach (if any object has debug info, suppress the warning). This probably makes real world applications less annoying since (crt1.o for exe) crtbegin.o crtend.o are linked into the binary and they typically don't have debug info. Then this appears to boil down to an existence check of .debug_line.

Sounds about right to me. Ideally, I'd still want to know if there was a dodgy piece of debug data somewhere which might be causing problems finding source information, but it's not that big a deal if the symbolizer code isn't plumbed up to give that information independently of simply "no debug data found for this location".

[MaskRay]

In D88715#2326883, @jhenderson wrote:

In D88715#2325275, @MaskRay wrote:

The internal user who reported the problem is fine with the approach (if any object has debug info, suppress the warning). This probably makes real world applications less annoying since (crt1.o for exe) crtbegin.o crtend.o are linked into the binary and they typically don't have debug info. Then this appears to boil down to an existence check of .debug_line.

Sounds about right to me. Ideally, I'd still want to know if there was a dodgy piece of debug data somewhere which might be causing problems finding source information, but it's not that big a deal if the symbolizer code isn't plumbed up to give that information independently of simply "no debug data found for this location".

OK. The warning feature requires more changes because symbolizeCode does not make "checkting whehter .debug_line exists" easy. I am not signing off on the work but maybe @mmpozulp can? :)

[MaskRay]

Ping:)

[jhenderson]

In D88715#2343345, @MaskRay wrote:

Ping:)

Is this ping directed specifically at @mmpozulp? I wasn't sure.

[MaskRay]

In D88715#2343696, @jhenderson wrote:

In D88715#2343345, @MaskRay wrote:

Ping:)

Is this ping directed specifically at @mmpozulp? I wasn't sure.

@mmpozulp may be inactive and then it is to you :) I think the warning code should be removed (like this patch does) until it has a less noisy implementation.

[jhenderson]

llvm/test/tools/llvm-objdump/X86/source-interleave-no-debug-info.test

8–9

[MaskRay]

Rebase.
Address comments.

[MaskRay]

In D88715#2343710, @MaskRay wrote:

In D88715#2343696, @jhenderson wrote:

In D88715#2343345, @MaskRay wrote:

Ping:)

Is this ping directed specifically at @mmpozulp? I wasn't sure.

@mmpozulp may be inactive and then it is to you :) I think the warning code should be removed (like this patch does) until it has a less noisy implementation. (It could be argued that such behavior may need to under an option.)

@jhenderson
Apologies that I did not see your previous test suggestion. Today I received another internal report that the failed to parse debug information warning is confusing when the executable has object file with no debug info (very common if we count system libraries and assembly object files).

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).

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.

[MaskRay]

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

29

This is not malformed input at all. Newer llc does not produce .debug_* if invalid filename (empty filename) is seen.

I have tried making .debug_* actually malformed but I don't see a warning. So the test can be improved, but it is out of the scope of this patch.

[jhenderson]

One small comment, then LGTM.

In 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.

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?

llvm/tools/llvm-objdump/llvm-objdump.cpp

1027–1028

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.

[MaskRay]

In D88715#2538614, @jhenderson wrote:

One small comment, then LGTM.

In 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.

Made a mistake in my previous comment. I intended to mean whether it is useful to emit a warning if all the objects were missing debug info.

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.

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?

Reopened https://bugs.llvm.org/show_bug.cgi?id=41905 with problems (parsing failures are not actually reported as warnings).

[MaskRay]

Drop duplicate filename in the message (which is currently a dead codepath)

[rupprecht]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1020–1021

auto -> Expected<DILineInfo> to be more readable

1029

Does removing the WarnedNoDebugInfo check mean that if debug info is present & malformed, we'll start displaying one message per line that the debug info is malformed?

[MaskRay]

Address comments

[rupprecht]

Looks good on my end now. Thanks!

[jhenderson]

LGTM too.

[jhenderson]

@MaskRay, I've resolved https://bugs.llvm.org/show_bug.cgi?id=47392, as I believe this indirectly fixes that issue. Can you confirm?

[jhenderson]

@MaskRay, do you think this fix would be useful to be in the LLVM 12 release? I think there's still time to get it cherry-picked.

[MaskRay]

In D88715#2568067, @jhenderson wrote:

@MaskRay, do you think this fix would be useful to be in the LLVM 12 release? I think there's still time to get it cherry-picked.

Good idea. Filed https://bugs.llvm.org/show_bug.cgi?id=49230

llvm/test/tools/llvm-objdump/X86/source-interleave-no-debug-info.test

 ## Test that if an object has no debug information, only the disassembly is
-## printed when --source is specified, and that we emit a warning.
+## printed when --source is specified, and that we do not emit a warning.
 
 # RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t.ll
 # RUN: llc -o %t.o -filetype=obj -mtriple=x86_64-pc-linux %t.ll
 # RUN: llvm-objcopy --strip-debug %t.o %t2.o
 
 # RUN: llvm-objdump --source %t.o | FileCheck %s --check-prefixes=CHECK,SOURCE
-# RUN: llvm-objdump --source %t2.o 2> %t2.e | FileCheck %s --check-prefixes=CHECK --implicit-check-not='main()'
-# RUN: FileCheck %s --input-file %t2.e --check-prefixes=WARN
+# RUN: llvm-objdump --source %t2.o 2>&1 | FileCheck %s --check-prefixes=CHECK --implicit-check-not='main()' --implicit-check-not=warning:
 
-# WARN:        warning: '{{.*}}2.o': failed to parse debug information
 # CHECK:       0000000000000010 <main>:
 # SOURCE-NEXT: ; int main() {
 # CHECK-NEXT:   10:   55                      pushq   %rbp

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

 
 ;; Test valid source interleave broken by adding an incorrect prefix.
 
 ; RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t-correct-prefix.ll
 ; RUN: llc -o %t-correct-prefix.o -filetype=obj -mtriple=x86_64-pc-linux %t-correct-prefix.ll
 ; RUN: llvm-objdump --prefix myprefix --source %t-correct-prefix.o 2>&1 | \
 ; RUN:   FileCheck %s --check-prefix=CHECK-BROKEN-PREFIX -DFILE=%t-correct-prefix.o -DPREFIX=myprefix%/p
 
-;; Test malformed input.
-
-; RUN: sed -e "s,SRC_COMPDIR,,g" -e "s,filename: \"source-interleave-x86_64.c\",filename: \"\",g" \
-; RUN:   %p/Inputs/source-interleave.ll > %t-malformed.ll
-; RUN: llc -o %t-malformed.o -filetype=obj -mtriple=x86_64-pc-linux %t-malformed.ll
-; RUN: llvm-objdump --prefix myprefix --source %t-malformed.o 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=CHECK-MALFORMED -DFILE=%t-malformed.o
-; CHECK-MALFORMED: warning: '[[FILE]]': failed to parse debug information for [[FILE]]
-
 ;; Using only a prefix separator is the same as not using the `--prefix` option.
 
 ; RUN: llvm-objdump --prefix / --source %t-missing-prefix.o 2>&1 | \
 ; RUN:   FileCheck %s --check-prefix=CHECK-BROKEN-PREFIX -DFILE=%t-missing-prefix.o -DPREFIX=''
 
 ;; All trailing separators on the prefix are discarded.
 ;; The prefix 'myprefix//' is converted to 'myprefix'.
 
 ; RUN: llvm-objdump --prefix myprefix// --source %t-missing-prefix.o 2>&1 | \
 ; RUN:   FileCheck %s --check-prefix=CHECK-BROKEN-PREFIX -DFILE=%t-missing-prefix.o -DPREFIX=myprefix

llvm/tools/llvm-objdump/llvm-objdump.cpp

protected:
   const ObjectFile *Obj = nullptr;
   std::unique_ptr<symbolize::LLVMSymbolizer> Symbolizer;
   // File name to file contents of source.
   std::unordered_map<std::string, std::unique_ptr<MemoryBuffer>> SourceCache;
   // Mark the line endings of the cached source.
   std::unordered_map<std::string, std::vector<StringRef>> LineCache;
   // Keep track of missing sources.
   StringSet<> MissingSources;
-  // Only emit 'no debug info' warning once.
-  bool WarnedNoDebugInfo;
+  // Only emit 'invalid debug info' warning once.
+  bool WarnedInvalidDebugInfo = false;
 
 private:
   bool cacheSource(const DILineInfo& LineInfoFile);
 
   void printLines(formatted_raw_ostream &OS, const DILineInfo &LineInfo,
                   StringRef Delimiter, LiveVariablePrinter &LVP);
 
   void printSources(formatted_raw_ostream &OS, const DILineInfo &LineInfo,
                     StringRef ObjectFilename, StringRef Delimiter,
                     LiveVariablePrinter &LVP);
 
 public:
   SourcePrinter() = default;
-  SourcePrinter(const ObjectFile *Obj, StringRef DefaultArch)
-      : Obj(Obj), WarnedNoDebugInfo(false) {
+  SourcePrinter(const ObjectFile *Obj, StringRef DefaultArch) : Obj(Obj) {
     symbolize::LLVMSymbolizer::Options SymbolizerOpts;
     SymbolizerOpts.PrintFunctions =
         DILineInfoSpecifier::FunctionNameKind::LinkageName;
     SymbolizerOpts.Demangle = Demangle;
     SymbolizerOpts.DefaultArch = std::string(DefaultArch);
     Symbolizer.reset(new symbolize::LLVMSymbolizer(SymbolizerOpts));
   }
   virtual ~SourcePrinter() = default;
void SourcePrinter::printSourceLine(formatted_raw_ostream &OS,
                                     object::SectionedAddress Address,
                                     StringRef ObjectFilename,
                                     LiveVariablePrinter &LVP,
                                     StringRef Delimiter) {
   if (!Symbolizer)
     return;
 
   DILineInfo LineInfo = DILineInfo();
-  auto ExpectedLineInfo = Symbolizer->symbolizeCode(*Obj, Address);
+  Expected<DILineInfo> ExpectedLineInfo =
+      Symbolizer->symbolizeCode(*Obj, Address);
   std::string ErrorMessage;
-  if (!ExpectedLineInfo)
-    ErrorMessage = toString(ExpectedLineInfo.takeError());
-  else
+  if (ExpectedLineInfo) {
     LineInfo = *ExpectedLineInfo;
-
-  if (LineInfo.FileName == DILineInfo::BadString) {
-    if (!WarnedNoDebugInfo) {
-      std::string Warning =
-          "failed to parse debug information for " + ObjectFilename.str();
-      if (!ErrorMessage.empty())
-        Warning += ": " + ErrorMessage;
-      reportWarning(Warning, ObjectFilename);
-      WarnedNoDebugInfo = true;
-    }
+  } else if (!WarnedInvalidDebugInfo) {
+    WarnedInvalidDebugInfo = true;
+    // TODO Untested.
+    reportWarning("failed to parse debug information: " +
+                      toString(ExpectedLineInfo.takeError()),
+                  ObjectFilename);
   }
 
   if (!Prefix.empty() && sys::path::is_absolute_gnu(LineInfo.FileName)) {
     SmallString<128> FilePath;
     sys::path::append(FilePath, Prefix, LineInfo.FileName);
 
     LineInfo.FileName = std::string(FilePath);
   }