Page MenuHomePhabricator

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

Authored by MaskRay on Oct 1 2020, 7:19 PM.

Details

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) as it stands is confusing (when there is no debug info it should not error for failed to parse).
This was brought up once on http://lists.llvm.org/pipermail/llvm-dev/2020-April/141264.html and an internal
user mentioned it to me that it was annoying. (I personally ignore the warning.)

This patch changes the warning to only fire when LLVMSymbolizer::symbolizeCode returns 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)

If we still need a warning for a binary with no debug info, we should do it properly.

Diff Detail

Unit TestsFailed

TimeTest
870 mslinux > libarcher.parallel::parallel-nosuppression.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-nosuppression.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/parallel/parallel-nosuppression.c

Event Timeline

MaskRay created this revision.Oct 1 2020, 7:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 7:19 PM
MaskRay requested review of this revision.Oct 1 2020, 7:19 PM
MaskRay updated this revision to Diff 295719.Oct 1 2020, 7:21 PM

Use Twine

Harbormaster completed remote builds in B73732: Diff 295719.
rupprecht added inline comments.Oct 2 2020, 10:15 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1018

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

1019–1021

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

1024–1030

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 added inline comments.Oct 2 2020, 10:42 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1018

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

1019–1021

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 added inline comments.Oct 2 2020, 11:07 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1018

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?

1019–1021

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

MaskRay added inline comments.Oct 2 2020, 11:42 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1018

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.

1019–1021

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.

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
1019–1021

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.

1024–1030

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 marked an inline comment as done.Oct 9 2020, 10:00 AM
MaskRay added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1019–1021

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

1024–1030

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 marked an inline comment as done.EditedOct 9 2020, 10:18 AM

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

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.

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.

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

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 edited the summary of this revision. (Show Details)Oct 14 2020, 2:35 PM

Ping:)

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

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 added inline comments.Oct 21 2020, 12:39 AM
llvm/test/tools/llvm-objdump/X86/source-interleave-no-debug-info.test
9–10