[PATCH] D70081: DWARFDebugLoclists: add location list "interpretation" logic

Pavel Labath via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 12 01:09:04 PST 2019


labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:61-63
+    Optional<SectionedAddress> LowPC = LookupAddr(E.Value0);
+    if (!LowPC)
+      return createResolverError(E.Value0, E.Kind);
----------------
dblaikie wrote:
> 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))
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).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:126
+        OS << "          => ";
+      Loc.get()->Range->dump(OS, Data.getAddressSize(), DumpOpts);
+    }
----------------
dblaikie wrote:
> 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)
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.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_loc_dwo.s:9
 # CHECK-NEXT:    0x00000000:
-# CHECK-NEXT:    Addr idx 1 (w/ length 16): DW_OP_reg5 RDI
+# CHECK-NEXT:    DW_LLE_startx_length (0x00000001, 0x00000010): DW_OP_reg5 RDI
 
----------------
SouraVX wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70081





More information about the llvm-commits mailing list