Page MenuHomePhabricator

DWARFDebugLoclists: add location list "interpretation" logic
ClosedPublic

Authored by labath on Nov 11 2019, 6:53 AM.

Details

Summary

This patch extracts the logic for computing the "absolute" locations,
which was partially present in the debug_loclists dumper, completes it,
and moves it into a separate function. This makes it possible to later
reuse the same logic for uses other than dumping.

The dumper is changed to reuse the location list interpreter, and its
format is changed somewhat. In "verbose" mode it prints the "raw" value
of a location list, the interpreted location (if available) and the
expression itself. In non-verbose mode it prints only one of the
location forms: it prefers the interpreted form, but falls back to the
"raw" format if interpretation is not possible (for instance, because we
were not given a base address, or the resolution of indirect addresses
failed).

This patch also undos some of the changes made in D69672, namely the
part about making all functions static. The main reason for this is that
I learned that the original approach (dumping only fully resolved
locations) meant that it was impossible to rewrite one of the existing
tests. To make that possible (and make the "inline location" dump work
in more cases), I now reuse the same dumping mechanism as is used for
section-based dumping. As this required having more objects know about
the various location lists classes, it seemed like a good idea to create
an interface abstracting the difference between them.

Therefore, I now create a DWARFLocationTable class, which will serve as
a base class for the location list classes. DWARFDebugLoclists is made
to inherit from that. DWARFDebugLoc will follow.

Another positive effect of this change is that section-based dumping
code will not need to use templates (as originally) envisioned, and that
the argument lists of the dumping functions become shorter.

Diff Detail

Event Timeline

labath created this revision.Nov 11 2019, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 6:53 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
labath marked an inline comment as done.Nov 11 2019, 6:57 AM
labath added inline comments.
llvm/test/DebugInfo/X86/loclists-dwp.ll
22–25

This is the test I was talking about above. It's sole purpose is to test handling of dwp files, but one cannot resolve addresses properly just by looking at the dwp file -- it doesn't have the address pool, and llvm-dwarfdump does not have the ability to provide it right now. By maintaining the ability to print _something_ in this case, we can keep the test, and make dumps of dwp/dwo files more useful.

dblaikie accepted this revision.Nov 11 2019, 4:14 PM
dblaikie added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
61–63

This error misses an opportunity to say why the indirect address resolution failed - was it because the section is missing entirely, or because the index provided is outside the range of the addresses present in the address pool. "nice to have/at some point" but if the mood strikes you/you see a nice way to support that better (Error<SectionedAddress> rather than Optional<SectionedAddress> from the lookup callback, I guess - it could be then stitched together - taking the stringified version of that error and adding it into a string that has the local context here, the LLE and anything else that might be useful (offset in the section, though that isn't preserved - no worries))

126

Future change: should we roll the DWARFFormValue::dumpAddressSection from DWARFDie.cpp::dumpRanges into DWARFAddressRange::dump to get the pretty printing of section name/number in here too?

(currently probably not super important - I don't think we ever produce location lists for globals, and locals are all in one function which is currently one section - but with Propeller it'd be possible to have a location list across multiple sections so dumping the section name/number would be useful then)

This revision is now accepted and ready to land.Nov 11 2019, 4:14 PM
SouraVX added inline comments.Nov 11 2019, 10:37 PM
llvm/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s
9

Hi Pavel, I'm not sure of this, here you're using "DW_LLE_startx_length" in a debug_loc.dwo -- any ideas ?
My primary concern here, is the Debuggability impact.
Though haven't, checked this? Did you get chance to debug these binaries{Containing above style location list} with LLDB or GDB ?? since predwarf(debug_loc and debug_loc.dwo) is debuggable with both debuggers.

labath marked 3 inline comments as done.Nov 12 2019, 12:58 AM
labath added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
61–63

I'll think about that. Right now that isn't my priority, as this error isn't even displayed in llvm-dwarfdump -- the reason for that is because it's tricky to figure out when displaying it "useful" (e.g. when dumping a dwo file, all attempts to resolve addresses would fail, and so the errors would just be noise).

126

Yes, I think we should do that. In fact, this is the reason I did not add the section index manipulation to more places yet (e.g. loclists parser) -- there's no way to test that now.

llvm/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s
9

I'm afraid I don't understand your concerns here. Can you elaborate?

This patch doesn't change how debug_loc.dwo sections are generated. It doesn't even change how they are parsed, for the most part. The only real difference in this test is that we now explicitly print the raw entry contents whereas previously we tried to "pretty print" it, which wasn't really useful without an address pool.

clang (and gcc, I believe) have been emitting this unstandardized .debug_loc.dwo format for a while. I haven't tried debugging these binaries lately, but I know that lldb has support for this format, but it is incomplete.

This revision was automatically updated to reflect the committed changes.
SouraVX added inline comments.Nov 12 2019, 10:24 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s
9

Thanks Pavel, for clarifying this. Indeed it's not touching the emission / generating part.
Obviously Debuggability is fine, checked with latest master.

This test is mostly about dwarfdump. Just curious here as to why, you've chosen "DW_LLE_*" representation here ?? those are [DWARF5] part.

dblaikie added inline comments.Nov 12 2019, 11:55 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s
9

The pre-standard/standard-proposal version of Fission used this same terminology: https://gcc.gnu.org/wiki/DebugFission so the names are suitable (& since they had no prior art in the standard there was no need to use an implementation namespace (DW_LLE_GNU_etc) first (well, kind of/might've been good to do that))

Now, I'm assuming "DW_LLE*" dumping/other stuff works fine with llvm-objdump and objdump GDB. since they share the same encoding's {pre/post DWARF5}?
But thier's one more slight glitch, for case of debug_loc{No split} section. We are still emitting raw ranges in a sense --

llvm-dwarfdump -debug-loc -v a.out   --using latest master
.debug_loc contents:
0x00000000:
            [0x0000000000000000,  0x0000000000000004): DW_OP_reg5 RDI
            [0x0000000000000004,  0x0000000000000018): DW_OP_reg3 RBX

Now I understand it's not priority, but atleast for sake of uniformity and saving some confusion. We should use it in both ways or completely drop it.
Minor addition -- objdump is still broken to handle [DWARF5] stuff. for debug_loc and loc.dwo it dumps raw ranges.

Now, I'm assuming "DW_LLE*" dumping/other stuff works fine with llvm-objdump and objdump GDB. since they share the same encoding's {pre/post DWARF5}?
But thier's one more slight glitch, for case of debug_loc{No split} section. We are still emitting raw ranges in a sense --

llvm-dwarfdump -debug-loc -v a.out   --using latest master
.debug_loc contents:
0x00000000:
            [0x0000000000000000,  0x0000000000000004): DW_OP_reg5 RDI
            [0x0000000000000004,  0x0000000000000018): DW_OP_reg3 RBX

Now I understand it's not priority, but atleast for sake of uniformity and saving some confusion. We should use it in both ways or completely drop it.
Minor addition -- objdump is still broken to handle [DWARF5] stuff. for debug_loc and loc.dwo it dumps raw ranges.

You're absolutely right, and I am planning to do that. I already have a patch which does just that -- I just need to clean it up before submission (I'll do that today or tomorrow).