[llvm-dwarfdump] - Fix incorrect parsing of the DW_LLE_startx_length

[grimar]

Summary

As was already mentioned in comments for D53364, DWARF 5 spec says about:

DW_LLE_startx_length: "This is a form of bounded location description that has two unsigned ULEB operands.
The first value is an address index (into the .debug_addr section) that indicates the beginning of the address range
over which the location is valid. The second value is the length of the range. ")

Currently, the length is parsed as U32, patch fixes the issue.

[grimar]

It turns out LLVM emits the U32 now:
https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1972

I'll prepare the patch for it too. Seems we will want to land these at the same time then?

Or should we handle DW_LLE_startx_length for .debug_loc (DWARF 4 dwo case) and .debug_loclists (DWARF5) differently for compatibility (I guess no as it does not match the specification)?

[dblaikie]

In D53564#1272488, @grimar wrote:

It turns out LLVM emits the U32 now:
https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1972

I'll prepare the patch for it too. Seems we will want to land these at the same time then?

Or should we handle DW_LLE_startx_length for .debug_loc (DWARF 4 dwo case) and .debug_loclists (DWARF5) differently for compatibility (I guess no as it does not match the specification)?

Yep, looks like this changed between the pre-standard definition ( https://gcc.gnu.org/wiki/DebugFission - "A start/length entry contains one unsigned LEB128 number and a 4-byte unsigned value (as would be represented by the form code DW_FORM_const4u). The first number is an index into the .debug_addr section that selects the beginning offset, and the second number is the length of the range. ") and the standard one (http://dwarfstd.org/doc/DWARF5.pdf - "This is a form of bounded location description that has two unsigned ULEB operands. The first value is an address index (into the .debug_addr section) that indicates the beginning of the address range over which the location is valid. The second value is the length of the range.")

So it should change based on DWARF version, unfortunately.

[aprantl]

In D53564#1272771, @dblaikie wrote:

So it should change based on DWARF version, unfortunately.

What happens when the llvm-dwarfdump with this patch reads the old format? Depending on the answer it might be useful to also add a --verify check for this. I'll leave this up to you.

[dblaikie]

In D53564#1272791, @aprantl wrote:

In D53564#1272771, @dblaikie wrote:

So it should change based on DWARF version, unfortunately.

What happens when the llvm-dwarfdump with this patch reads the old format? Depending on the answer it might be useful to also add a --verify check for this. I'll leave this up to you.

That's what I mean by "should change based on DWARF version" - this patch, as-is, is insufficient (since it'd break parsing pre-standard split-DWARF).

Not sure what --verify could do about this - since the data isn't self-describing, bytes are bytes and these rules specify how to interpret those bytes. If that makes sense?

[aprantl]

In D53564#1272837, @dblaikie wrote:

In D53564#1272791, @aprantl wrote:

In D53564#1272771, @dblaikie wrote:

So it should change based on DWARF version, unfortunately.

What happens when the llvm-dwarfdump with this patch reads the old format? Depending on the answer it might be useful to also add a --verify check for this. I'll leave this up to you.

That's what I mean by "should change based on DWARF version" - this patch, as-is, is insufficient (since it'd break parsing pre-standard split-DWARF).

Agreed.

Not sure what --verify could do about this - since the data isn't self-describing, bytes are bytes and these rules specify how to interpret those bytes. If that makes sense?

You're right. This is an abbrev entry; just raw data.

[grimar]

• Parse DW_LLE_startx_length differently for v4/pre-v5 and v5 sections.

[dblaikie]

Looks good, thanks!

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h

private:
 
   LocationLists Locations;
 
   unsigned AddressSize;
 
   bool IsLittleEndian;
 
 public:
-  void parse(DataExtractor data);
+  void parse(DataExtractor data, unsigned Version);
   void dump(raw_ostream &OS, uint64_t BaseAddr, const MCRegisterInfo *RegInfo,
             Optional<uint64_t> Offset) const;
 
   /// Return the location list at the given offset or nullptr.
   LocationList const *getLocationListAtOffset(uint64_t Offset) const;
 
-  static Optional<LocationList> parseOneLocationList(DataExtractor Data,
-                                                     uint32_t *Offset);
+  static Optional<LocationList>
+  parseOneLocationList(DataExtractor Data, unsigned *Offset, unsigned Version);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGLOC_H

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h

public:
 
   void clear() {
     HeaderData = {};
     Offsets.clear();
   }
   uint32_t getHeaderOffset() const { return HeaderOffset; }
   uint8_t getAddrSize() const { return HeaderData.AddrSize; }
   uint32_t getLength() const { return HeaderData.Length; }
+  uint16_t getVersion() const { return HeaderData.Version; }
   StringRef getSectionName() const { return SectionName; }
   StringRef getListTypeString() const { return ListTypeString; }
   dwarf::DwarfFormat getFormat() const { return Format; }
 
   void dump(raw_ostream &OS, DIDumpOptions DumpOpts = {}) const;
   Optional<uint32_t> getOffsetEntry(uint32_t Index) const {
     if (Index < Offsets.size())
       return Offsets[Index];

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

if (Error E = Header.extract(Data, &Offset)) {
     WithColor::error() << toString(std::move(E)) << '\n';
     return;
   }
 
   Header.dump(OS, DumpOpts);
   DataExtractor LocData(Data.getData().drop_front(Offset),
                         Data.isLittleEndian(), Header.getAddrSize());
 
-  Loclists.parse(LocData);
+  Loclists.parse(LocData, Header.getVersion());
   Loclists.dump(OS, 0, MRI, DumpOffset);
 }
 
 void DWARFContext::dump(
     raw_ostream &OS, DIDumpOptions DumpOpts,
     std::array<Optional<uint64_t>, DIDT_ID_Count> DumpOffsets) {
 
   Optional<uint64_t> DumpOffset;
const DWARFDebugLoclists *DWARFContext::getDebugLocDWO() {
   if (LocDWO)
     return LocDWO.get();
 
   LocDWO.reset(new DWARFDebugLoclists());
   // Assume all compile units have the same address byte size.
   // FIXME: We don't need AddressSize for split DWARF since relocatable
   // addresses cannot appear there. At the moment DWARFExpression requires it.
   DataExtractor LocData(DObj->getLocDWOSection().Data, isLittleEndian(), 4);
-  LocDWO->parse(LocData);
+  // Use version 4. DWO does not support the DWARF v5 .debug_loclists yet and
+  // that means we are parsing the new style .debug_loc (pre-standatized version
+  // of the .debug_loclists).
+  LocDWO->parse(LocData, 4 /* Version */);
   return LocDWO.get();
 }
 
 const DWARFDebugAranges *DWARFContext::getDebugAranges() {
   if (Aranges)
     return Aranges.get();
 
   Aranges.reset(new DWARFDebugAranges());

llvm/trunk/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

while (data.isValidOffset(Offset + data.getAddressSize() - 1)) {
     else
       break;
   }
   if (data.isValidOffset(Offset))
     WithColor::error() << "failed to consume entire .debug_loc section\n";
 }
 
 Optional<DWARFDebugLoclists::LocationList>
-DWARFDebugLoclists::parseOneLocationList(DataExtractor Data, unsigned *Offset) {
+DWARFDebugLoclists::parseOneLocationList(DataExtractor Data, unsigned *Offset,
+                                         unsigned Version) {
   LocationList LL;
   LL.Offset = *Offset;
 
   // dwarf::DW_LLE_end_of_list_entry is 0 and indicates the end of the list.
   while (auto Kind =
              static_cast<dwarf::LocationListEntry>(Data.getU8(Offset))) {
 
     Entry E;
     E.Kind = Kind;
     switch (Kind) {
     case dwarf::DW_LLE_startx_length:
       E.Value0 = Data.getULEB128(Offset);
+      // Pre-DWARF 5 has different interpretation of the length field. We have
+      // to support both pre- and standartized styles for the compatibility.
+      if (Version < 5)
         E.Value1 = Data.getU32(Offset);
+      else
+        E.Value1 = Data.getULEB128(Offset);
       break;
     case dwarf::DW_LLE_start_length:
       E.Value0 = Data.getAddress(Offset);
       E.Value1 = Data.getULEB128(Offset);
       break;
     case dwarf::DW_LLE_offset_pair:
       E.Value0 = Data.getULEB128(Offset);
       E.Value1 = Data.getULEB128(Offset);
while (auto Kind =
     E.Loc.resize(str.size());
     std::copy(str.begin(), str.end(), E.Loc.begin());
 
     LL.Entries.push_back(std::move(E));
   }
   return LL;
 }
 
-void DWARFDebugLoclists::parse(DataExtractor data) {
+void DWARFDebugLoclists::parse(DataExtractor data, unsigned Version) {
   IsLittleEndian = data.isLittleEndian();
   AddressSize = data.getAddressSize();
 
   uint32_t Offset = 0;
   while (data.isValidOffset(Offset)) {
-    if (auto LL = parseOneLocationList(data, &Offset))
+    if (auto LL = parseOneLocationList(data, &Offset, Version))
       Locations.push_back(std::move(*LL));
     else
       return;
   }
 }
 
 DWARFDebugLoclists::LocationList const *
 DWARFDebugLoclists::getLocationListAtOffset(uint64_t Offset) const {

llvm/trunk/lib/DebugInfo/DWARF/DWARFDie.cpp

if (!U->isDWOUnit() && !U->getLocSection()->Data.empty()) {
           BaseAddr = BA->Address;
         LL->dump(OS, Ctx.isLittleEndian(), Obj.getAddressSize(), MRI, BaseAddr,
                  Indent);
       } else
         OS << "error extracting location list.";
       return;
     }
 
+    bool UseLocLists = !U->isDWOUnit();
     StringRef LoclistsSectionData =
-        U->isDWOUnit() ? U->getLocSectionData() : Obj.getLoclistsSection().Data;
+        UseLocLists ? Obj.getLoclistsSection().Data : U->getLocSectionData();
+
     if (!LoclistsSectionData.empty()) {
       DataExtractor Data(LoclistsSectionData, Ctx.isLittleEndian(),
                          Obj.getAddressSize());
-      auto LL = DWARFDebugLoclists::parseOneLocationList(Data, &Offset);
+
+      // Old-style location list were used in DWARF v4 (.debug_loc.dwo section).
+      // Modern locations list (.debug_loclists) are used starting from v5.
+      // Ideally we should take the version from the .debug_loclists section
+      // header, but using CU's version for simplicity.
+      auto LL = DWARFDebugLoclists::parseOneLocationList(
+          Data, &Offset, UseLocLists ? U->getVersion() : 4);
 
       uint64_t BaseAddr = 0;
       if (Optional<SectionedAddress> BA = U->getBaseAddress())
         BaseAddr = BA->Address;
 
       if (LL)
         LL->dump(OS, BaseAddr, Ctx.isLittleEndian(), Obj.getAddressSize(), MRI,
                  Indent);

llvm/trunk/test/tools/llvm-dwarfdump/X86/debug_loclists_startx_length.s

+# RUN: llvm-mc %s -filetype obj -triple x86_64-pc-linux -o %t.o
+# RUN: llvm-dwarfdump -v %t.o | FileCheck %s
+
+# DW_LLE_startx_length has different `length` encoding in pre-DWARF 5
+# and final DWARF 5 versions. This test checks we are able to parse
+# the final version which uses ULEB128 and not the U32.
+
+# CHECK:         .debug_loclists contents:
+# CHECK-NEXT:    0x00000000: locations list header: length = 0x0000000f, version = 0x0005, addr_size = 0x08, seg_size = 0x00, offset_entry_count = 0x00000000
+# CHECK-NEXT:    0x00000000:
+# CHECK-NEXT:    Addr idx 1 (w/ length 16): DW_OP_reg5 RDI
+
+.section .debug_loclists,"",@progbits
+ .long  .Ldebug_loclist_table_end0-.Ldebug_loclist_table_start0
+.Ldebug_loclist_table_start0:
+ .short 5         # Version.
+ .byte 8          # Address size.
+ .byte 0          # Segmen selector size.
+ .long 0          # Offset entry count.
+ 
+ .byte 3          # DW_LLE_startx_length
+ .byte 0x01       # Index
+ .uleb128 0x10    # Length
+ .short 1         # Loc expr size
+ .byte 85         # DW_OP_reg5
+ .byte 0          # DW_LLE_end_of_list
+.Ldebug_loclist_table_end0: