DWARFDebugLoclists: Move to a incremental parsing model

[labath]

Summary

This patch stems from the discussion D68270 (including some offline
talks). The idea is to provide an "incremental" api for parsing location
lists, which will avoid caching or materializing parsed data. An
additional goal is to provide a high level location list api, which
abstracts the differences between different encoding schemes, and can be
used by users which don't care about those (such as LLDB).

This patch implements the first part. It implements a call-back based
"visitLocationList" api. This function parses a single location list,
calling a user-specified callback for each entry. This is going to be
the base api, which other location list functions (right now, just the
dumping code) are going to be based on.

Future patches will do something similar for the v4 location lists, and
add a mechanism to translate raw entries into concrete address ranges.

[labath]

It looks like there will be some conflicts between this patch and D69462, but it does not seem they should be hard to resolve, as that patch mostly deals with the generation side of things.

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

82

Something which isn't obvious from this patch is that our idea was to make the Entry class be shared between the two location list implementations. The v4 entries will get "upgraded" to v5 during parsing. This will make it possible to share the code translating the location list entries into actual address ranges. Dumping will be handled specially, in order to make the v4 dump come out as it was originally in the file.

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

210–219

Parsing twice just to get the maximum name length is somewhat wasteful. A different option would be to get the maximum of *all* entry types, not just of those that happen to be used in this location lists.

llvm/test/DebugInfo/X86/fission-ranges.ll

35–39

This is +/- the only functional change in this patch. It is unfortunate because the range list code does not print the extra colon. It wouldn't be *too* hard to avoid that. I'd just need to move the offset printing code out of the "dumpLocationList" function and into the individual (about 3 of them) callers. However, that did not seem worth it, particularly as I am hoping that this can be rectified once we have a more high-level method of printing "inline" location lists.

[labath]

Add some comments for the new functions.

[dblaikie]

Looks good!

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

210–219

Yeah, I'd be happy if you switch that to max of all entry types.

323

isValidOffset might be unnecessary given "Offset < EndOffset"? (maybe check that EndOffset is smaller than Data's actual end ofset to start with, then skip the isValidOffset check? (maybe need a lower bound check too))

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

121

@labath , Not sure about this piece of code, is this patch has some relationship with D68271 ??
I noticed this piece of code their, and which is uncommitted ??

[labath]

Will address @dblaikie's comments tomorrow.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

121

Yes, it is based on that patch, mainly because I was too lazy to create a new branch for this work. What is it that you are unsure of? (Is it just it's presence, or something more?)

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

121

Just the presence here(when I was rebaseing), I wasn't sure it's stemming / related from that patch.
I believe you must've forgotten to specified it's dependence / related to D68271. ??
Thanks for clarifying this.

[labath]

• change how maximum length is computed
• tweak end offset handling

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

323

This is debatable, I think. In case the EndOffset is wrong (corrupted header?), it may make sense to still dump as much as we are able to do instead of bailing out straight away. However, that is probably not a likely scenario, and it does make things cleaner, so I've gone with your suggestion. I've also changed the EndOffset argument into a "Size", as that seemed a bit cleaner.

The thing I would really want to have some day is the ability to "slice" a data extractor. That would make a lot of these offset calculations simpler and/or safer. For instance, this code has the bug that if one location list crosses the "end offset" boundary (and there is more data after it), then we will only detect that once we've finished dumping that location list. If the size was encoded in the data extractor, then this could be detected immediately, as each contribution would be handled as if it was the last one in the section.

[JDevlieghere]

Personally I'm not a big fan of callbacks, but it seems like this was well thought through. My original suggestion of doing a two-step parse also wouldn't allow you to abort parsing, which is possible here by returning false form the callback.

LGTM

[labath]

In D69672#1734182, @JDevlieghere wrote:

Personally I'm not a big fan of callbacks, but it seems like this was well thought through. My original suggestion of doing a two-step parse also wouldn't allow you to abort parsing, which is possible here by returning false form the callback.

Ah, I now fully understand what you meant on the other diff. Yes, being able to abort parsing part-way (along avoiding materializing the full list) is one of the benefits of this approach -- or at least, it makes it fit well into the way it is currently used from lldb. Though in practice, I don't expect that to matter much, as individual location lists are small [citation needed]. I'm not a fan of callbacks either, and if this had/will have more users, I'd go for turning these into iterators -- it's just that writing iterators is cumbersome (when can we have coroutines?), so it's not clear if it's worth the trouble.

LGTM

Thanks.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

class DWARFContext : public DIContext {
   std::unique_ptr<DWARFDebugNames> Names;
   std::unique_ptr<AppleAcceleratorTable> AppleNames;
   std::unique_ptr<AppleAcceleratorTable> AppleTypes;
   std::unique_ptr<AppleAcceleratorTable> AppleNamespaces;
   std::unique_ptr<AppleAcceleratorTable> AppleObjC;
 
   DWARFUnitVector DWOUnits;
   std::unique_ptr<DWARFDebugAbbrev> AbbrevDWO;
-  std::unique_ptr<DWARFDebugLoclists> LocDWO;
 
   /// The maximum DWARF version of all units.
   unsigned MaxVersion = 0;
 
   struct DWOFile {
     object::OwningBinary<object::ObjectFile> File;
     std::unique_ptr<DWARFContext> Context;
   };
public:
   const DWARFDebugAbbrev *getDebugAbbrev();
 
   /// Get a pointer to the parsed DebugLoc object.
   const DWARFDebugLoc *getDebugLoc();
 
   /// Get a pointer to the parsed dwo abbreviations object.
   const DWARFDebugAbbrev *getDebugAbbrevDWO();
 
-  /// Get a pointer to the parsed DebugLoc object.
-  const DWARFDebugLoclists *getDebugLocDWO();
-
   /// Get a pointer to the parsed DebugAranges object.
   const DWARFDebugAranges *getDebugAranges();
 
   /// Get a pointer to the parsed frame information object.
   const DWARFDebugFrame *getDebugFrame();
 
   /// Get a pointer to the parsed eh frame information object.
   const DWARFDebugFrame *getEHFrame();

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

public:
   LocationList const *getLocationListAtOffset(uint64_t Offset) const;
 
   Expected<LocationList>
   parseOneLocationList(const DWARFDataExtractor &Data, uint64_t *Offset);
 };
 
 class DWARFDebugLoclists {
 public:
+  // Unconstructible.
+  DWARFDebugLoclists() = delete;
+
   struct Entry {
     uint8_t Kind;
     uint64_t Offset;
     uint64_t Value0;
     uint64_t Value1;
     SmallVector<uint8_t, 4> Loc;
     void dump(raw_ostream &OS, uint64_t &BaseAddr, bool IsLittleEndian,
               unsigned AddressSize, const MCRegisterInfo *MRI, DWARFUnit *U,
               DIDumpOptions DumpOpts, unsigned Indent, size_t MaxEncodingStringLength) const;
   };
 
-  struct LocationList {
-    uint64_t Offset;
-    SmallVector<Entry, 2> Entries;
-    void dump(raw_ostream &OS, uint64_t BaseAddr, bool IsLittleEndian,
-              unsigned AddressSize, const MCRegisterInfo *RegInfo,
-              DWARFUnit *U, DIDumpOptions DumpOpts, unsigned Indent) const;
-  };
-
-private:
-  using LocationLists = SmallVector<LocationList, 4>;
-
-  LocationLists Locations;
-
-  unsigned AddressSize;
-
-  bool IsLittleEndian;
-
-public:
-  void parse(const DWARFDataExtractor &data, uint64_t Offset,
-             uint64_t EndOffset, uint16_t Version);
-  void dump(raw_ostream &OS, uint64_t BaseAddr, const MCRegisterInfo *RegInfo,
-            DIDumpOptions DumpOpts, Optional<uint64_t> Offset) const;
-
-  /// Return the location list at the given offset or nullptr.
-  LocationList const *getLocationListAtOffset(uint64_t Offset) const;
-
-  static Expected<LocationList>
-  parseOneLocationList(const DWARFDataExtractor &Data, uint64_t *Offset,
-                       unsigned Version);
+  /// Call the user-provided callback for each entry (including the end-of-list
+  /// entry) in the location list starting at \p Offset. The callback can return
+  /// false to terminate the iteration early. Returns an error if it was unable
+  /// to parse the entire location list correctly. Upon successful termination
+  /// \p Offset will be updated point past the end of the list.
+  static Error visitLocationList(const DWARFDataExtractor &Data,
+                                 uint64_t *Offset, uint16_t Version,
+                                 llvm::function_ref<bool(const Entry &)> F);
+
+  /// Dump the location list at the given \p Offset. The function returns true
+  /// iff it has successfully reched the end of the list. This means that one
+  /// can attempt to parse another list after the current one (\p Offset will be
+  /// updated to point past the end of the current list).
+  static bool dumpLocationList(const DWARFDataExtractor &Data, uint64_t *Offset,
+                               uint16_t Version, raw_ostream &OS,
+                               uint64_t BaseAddr, const MCRegisterInfo *MRI,
+                               DWARFUnit *U, DIDumpOptions DumpOpts,
+                               unsigned Indent);
+
+  /// Dump all location lists within the given range.
+  static void dumpRange(const DWARFDataExtractor &Data, uint64_t StartOffset,
+                        uint64_t Size, uint16_t Version, raw_ostream &OS,
+                        uint64_t BaseAddr, const MCRegisterInfo *MRI,
+                        DIDumpOptions DumpOpts);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGLOC_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

while (Data.isValidOffset(Offset)) {
     DWARFListTableHeader Header(".debug_loclists", "locations");
     if (Error E = Header.extract(Data, &Offset)) {
       WithColor::error() << toString(std::move(E)) << '\n';
       return;
     }
 
     Header.dump(OS, DumpOpts);
 
-    DWARFDebugLoclists Loclists;
     uint64_t EndOffset = Header.length() + Header.getHeaderOffset();
     Data.setAddressSize(Header.getAddrSize());
-    Loclists.parse(Data, Offset, EndOffset, Header.getVersion());
-    Loclists.dump(OS, 0, MRI, DumpOpts, DumpOffset);
+    if (DumpOffset) {
+      if (DumpOffset >= Offset && DumpOffset < EndOffset) {
+        Offset = *DumpOffset;
+        DWARFDebugLoclists::dumpLocationList(Data, &Offset, Header.getVersion(),
+                                             OS, /*BaseAddr=*/0, MRI, nullptr,
+                                             DumpOpts, /*Indent=*/0);
+        OS << "\n";
+        return;
+      }
+    } else {
+      DWARFDebugLoclists::dumpRange(Data, Offset, EndOffset - Offset,
+                                    Header.getVersion(), OS, 0, MRI, DumpOpts);
+    }
     Offset = EndOffset;
   }
 }
 
 void DWARFContext::dump(
     raw_ostream &OS, DIDumpOptions DumpOpts,
     std::array<Optional<uint64_t>, DIDT_ID_Count> DumpOffsets) {
 
if (const auto *Off =
                      DObj->getLoclistsSection().Data)) {
     DWARFDataExtractor Data(*DObj, DObj->getLoclistsSection(), isLittleEndian(),
                             0);
     dumpLoclistsSection(OS, DumpOpts, Data, getRegisterInfo(), *Off);
   }
   if (const auto *Off =
           shouldDump(ExplicitDWO, ".debug_loc.dwo", DIDT_ID_DebugLoc,
                      DObj->getLocDWOSection().Data)) {
-    getDebugLocDWO()->dump(OS, 0, getRegisterInfo(), DumpOpts, *Off);
+    DWARFDataExtractor Data(*DObj, DObj->getLocDWOSection(), isLittleEndian(),
+                            4);
+    if (*Off) {
+      uint64_t Offset = **Off;
+      DWARFDebugLoclists::dumpLocationList(Data, &Offset, /*Version=*/4, OS,
+                                           /*BaseAddr=*/0, getRegisterInfo(),
+                                           nullptr, DumpOpts, /*Indent=*/0);
+      OS << "\n";
+    } else {
+      DWARFDebugLoclists::dumpRange(Data, 0, Data.getData().size(),
+                                    /*Version=*/4, OS, /*BaseAddr=*/0,
+                                    getRegisterInfo(), DumpOpts);
+    }
   }
 
   if (const auto *Off = shouldDump(Explicit, ".debug_frame", DIDT_ID_DebugFrame,
                                    DObj->getFrameSection().Data))
     getDebugFrame()->dump(OS, getRegisterInfo(), *Off);
 
   if (const auto *Off = shouldDump(Explicit, ".eh_frame", DIDT_ID_DebugFrame,
                                    DObj->getEHFrameSection().Data))
const DWARFDebugLoc *DWARFContext::getDebugLoc() {
   if (getNumCompileUnits()) {
     DWARFDataExtractor LocData(*DObj, DObj->getLocSection(), isLittleEndian(),
                                getUnitAtIndex(0)->getAddressByteSize());
     Loc->parse(LocData);
   }
   return Loc.get();
 }
 
-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.
-  DWARFDataExtractor LocData(*DObj, DObj->getLocDWOSection(), isLittleEndian(),
-                             4);
-  // 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, 0, LocData.getData().size(), 4 /* Version */);
-  return LocDWO.get();
-}
-
 const DWARFDebugAranges *DWARFContext::getDebugAranges() {
   if (Aranges)
     return Aranges.get();
 
   Aranges.reset(new DWARFDebugAranges());
   Aranges->generate(this);
   return Aranges.get();
 }

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

if (auto LL = parseOneLocationList(data, &Offset))
       Locations.push_back(std::move(*LL));
     else {
       logAllUnhandledErrors(LL.takeError(), WithColor::error());
       break;
     }
   }
 }
 
-Expected<DWARFDebugLoclists::LocationList>
-DWARFDebugLoclists::parseOneLocationList(const DWARFDataExtractor &Data,
-                                         uint64_t *Offset, unsigned Version) {
-  LocationList LL;
-  LL.Offset = *Offset;
-  DataExtractor::Cursor C(*Offset);
+Error DWARFDebugLoclists::visitLocationList(
+    const DWARFDataExtractor &Data, uint64_t *Offset, uint16_t Version,
+    llvm::function_ref<bool(const Entry &)> F) {
 
-  // dwarf::DW_LLE_end_of_list_entry is 0 and indicates the end of the list.
-  while (auto Kind = Data.getU8(C)) {
+  DataExtractor::Cursor C(*Offset);
+  bool Continue = true;
+  while (Continue) {
     Entry E;
-    E.Kind = Kind;
-    E.Offset = C.tell() - 1;
-    switch (Kind) {
+    E.Offset = C.tell();
+    E.Kind = Data.getU8(C);
+    switch (E.Kind) {
+    case dwarf::DW_LLE_end_of_list:
+      break;
     case dwarf::DW_LLE_base_addressx:
       E.Value0 = Data.getULEB128(C);
       break;
     case dwarf::DW_LLE_startx_length:
       E.Value0 = Data.getULEB128(C);
       // 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(C);
       else
         E.Value1 = Data.getULEB128(C);
       break;
-    case dwarf::DW_LLE_start_length:
-      E.Value0 = Data.getRelocatedAddress(C);
-      E.Value1 = Data.getULEB128(C);
-      break;
     case dwarf::DW_LLE_offset_pair:
       E.Value0 = Data.getULEB128(C);
       E.Value1 = Data.getULEB128(C);
       break;
     case dwarf::DW_LLE_base_address:
       E.Value0 = Data.getRelocatedAddress(C);
       break;
+    case dwarf::DW_LLE_start_length:
+      E.Value0 = Data.getRelocatedAddress(C);
+      E.Value1 = Data.getULEB128(C);
+      break;
+    case dwarf::DW_LLE_startx_endx:
+    case dwarf::DW_LLE_default_location:
+    case dwarf::DW_LLE_start_end:
     default:
       cantFail(C.takeError());
       return createStringError(errc::illegal_byte_sequence,
-                               "LLE of kind %x not supported", (int)Kind);
+                               "LLE of kind %x not supported", (int)E.Kind);
     }
 
-    if (Kind != dwarf::DW_LLE_base_address &&
-        Kind != dwarf::DW_LLE_base_addressx) {
+    if (E.Kind != dwarf::DW_LLE_base_address &&
+        E.Kind != dwarf::DW_LLE_base_addressx &&
+        E.Kind != dwarf::DW_LLE_end_of_list) {
       unsigned Bytes = Version >= 5 ? Data.getULEB128(C) : Data.getU16(C);
       // A single location description describing the location of the object...
       Data.getU8(C, E.Loc, Bytes);
     }
 
-    LL.Entries.push_back(std::move(E));
+    if (!C)
+      return C.takeError();
+    Continue = F(E) && E.Kind != dwarf::DW_LLE_end_of_list;
   }
-  if (Error Err = C.takeError())
-    return std::move(Err);
-  Entry E;
-  E.Kind = dwarf::DW_LLE_end_of_list;
-  E.Offset = C.tell() - 1;
-  LL.Entries.push_back(E);
   *Offset = C.tell();
-  return LL;
+  return Error::success();
 }
 
-void DWARFDebugLoclists::parse(const DWARFDataExtractor &data, uint64_t Offset,
-                               uint64_t EndOffset, uint16_t Version) {
-  IsLittleEndian = data.isLittleEndian();
-  AddressSize = data.getAddressSize();
-
-  while (Offset < EndOffset) {
-    if (auto LL = parseOneLocationList(data, &Offset, Version))
-      Locations.push_back(std::move(*LL));
-    else {
-      logAllUnhandledErrors(LL.takeError(), WithColor::error());
-      return;
-    }
-  }
+bool DWARFDebugLoclists::dumpLocationList(const DWARFDataExtractor &Data,
+                                          uint64_t *Offset, uint16_t Version,
+                                          raw_ostream &OS, uint64_t BaseAddr,
+                                          const MCRegisterInfo *MRI,
+                                          DWARFUnit *U, DIDumpOptions DumpOpts,
+                                          unsigned Indent) {
+  size_t MaxEncodingStringLength = 0;
+  if (DumpOpts.Verbose) {
+#define HANDLE_DW_LLE(ID, NAME)                                                \
+  MaxEncodingStringLength = std::max(MaxEncodingStringLength,                  \
+                                     dwarf::LocListEncodingString(ID).size());
+#include "llvm/BinaryFormat/Dwarf.def"
   }
 
-DWARFDebugLoclists::LocationList const *
-DWARFDebugLoclists::getLocationListAtOffset(uint64_t Offset) const {
-  auto It = partition_point(
-      Locations, [=](const LocationList &L) { return L.Offset < Offset; });
-  if (It != Locations.end() && It->Offset == Offset)
-    return &(*It);
-  return nullptr;
+  OS << format("0x%8.8" PRIx64 ": ", *Offset);
+  Error E = visitLocationList(Data, Offset, Version, [&](const Entry &E) {
+    E.dump(OS, BaseAddr, Data.isLittleEndian(), Data.getAddressSize(), MRI, U,
+           DumpOpts, Indent, MaxEncodingStringLength);
+    return true;
+  });
+  if (E) {
+    OS << "\n";
+    OS.indent(Indent);
+    OS << "error: " << toString(std::move(E));
+    return false;
+  }
+  return true;
 }
 
 void DWARFDebugLoclists::Entry::dump(raw_ostream &OS, uint64_t &BaseAddr,
                                      bool IsLittleEndian, unsigned AddressSize,
                                      const MCRegisterInfo *MRI, DWARFUnit *U,
                                      DIDumpOptions DumpOpts, unsigned Indent,
                                      size_t MaxEncodingStringLength) const {
   if (DumpOpts.Verbose) {
if (!DumpOpts.Verbose)
       return;
     break;
   default:
     llvm_unreachable("unreachable locations list kind");
   }
 
   dumpExpression(OS, Loc, IsLittleEndian, AddressSize, MRI, U);
 }
-void DWARFDebugLoclists::LocationList::dump(raw_ostream &OS, uint64_t BaseAddr,
-                                            bool IsLittleEndian,
-                                            unsigned AddressSize,
-                                            const MCRegisterInfo *MRI,
-                                            DWARFUnit *U,
-                                            DIDumpOptions DumpOpts, 
-                                            unsigned Indent) const {
-  size_t MaxEncodingStringLength = 0;
-  if (DumpOpts.Verbose)
-    for (const auto &Entry : Entries)
-      MaxEncodingStringLength =
-          std::max(MaxEncodingStringLength,
-                   dwarf::LocListEncodingString(Entry.Kind).size());
-
-  for (const Entry &E : Entries)
-    E.dump(OS, BaseAddr, IsLittleEndian, AddressSize, MRI, U, DumpOpts, Indent,
-           MaxEncodingStringLength);
-}
 
-void DWARFDebugLoclists::dump(raw_ostream &OS, uint64_t BaseAddr,
-                              const MCRegisterInfo *MRI, DIDumpOptions DumpOpts, 
-                              Optional<uint64_t> Offset) const {
-  auto DumpLocationList = [&](const LocationList &L) {
-    OS << format("0x%8.8" PRIx64 ": ", L.Offset);
-    L.dump(OS, BaseAddr, IsLittleEndian, AddressSize, MRI, nullptr, DumpOpts,
-           /*Indent=*/12);
-    OS << "\n";
-  };
-
-  if (Offset) {
-    if (auto *L = getLocationListAtOffset(*Offset))
-      DumpLocationList(*L);
+void DWARFDebugLoclists::dumpRange(const DWARFDataExtractor &Data,
+                                   uint64_t StartOffset, uint64_t Size,
+                                   uint16_t Version, raw_ostream &OS,
+                                   uint64_t BaseAddr, const MCRegisterInfo *MRI,
+                                   DIDumpOptions DumpOpts) {
+  if (!Data.isValidOffsetForDataOfSize(StartOffset, Size))  {
+    OS << "Invalid dump range\n";
     return;
   }
+  uint64_t Offset = StartOffset;
+  StringRef Separator;
+  bool CanContinue = true;
+  while (CanContinue && Offset < StartOffset + Size) {
+    OS << Separator;
+    Separator = "\n";
 
-  for (const LocationList &L : Locations) {
-    DumpLocationList(L);
-    if (&L != &Locations.back())
-      OS << '\n';
+    CanContinue = dumpLocationList(Data, &Offset, Version, OS, BaseAddr, MRI,
+                                   nullptr, DumpOpts, /*Indent=*/12);
   }
 }

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

if (FormValue.isFormClass(DWARFFormValue::FC_Block) ||
     ArrayRef<uint8_t> Expr = *FormValue.getAsBlock();
     DataExtractor Data(StringRef((const char *)Expr.data(), Expr.size()),
                        Ctx.isLittleEndian(), 0);
     DWARFExpression(Data, U->getVersion(), U->getAddressByteSize())
         .print(OS, MRI, U);
     return;
   }
 
-  FormValue.dump(OS, DumpOpts);
-  const auto &DumpLL = [&](auto ExpectedLL) {
-    if (ExpectedLL) {
-      uint64_t BaseAddr = 0;
-      if (Optional<object::SectionedAddress> BA = U->getBaseAddress())
-        BaseAddr = BA->Address;
-      auto LLDumpOpts = DumpOpts;
-      LLDumpOpts.Verbose = false;
-      ExpectedLL->dump(OS, BaseAddr, Ctx.isLittleEndian(), Obj.getAddressSize(),
-                       MRI, U, LLDumpOpts, Indent);
+  if (FormValue.isFormClass(DWARFFormValue::FC_SectionOffset)) {
+    uint64_t Offset = *FormValue.getAsSectionOffset();
+
+    if (!U->isDWOUnit() && !U->getLocSection()->Data.empty()) {
+      DWARFDebugLoc DebugLoc;
+      DWARFDataExtractor Data(Obj, *U->getLocSection(), Ctx.isLittleEndian(),
+                              Obj.getAddressSize());
+
+      FormValue.dump(OS, DumpOpts);
+      OS << ": ";
+
+      if (Expected<DWARFDebugLoc::LocationList> LL =
+              DebugLoc.parseOneLocationList(Data, &Offset)) {
+        LL->dump(OS, BaseAddr, Ctx.isLittleEndian(), Obj.getAddressSize(), MRI,
+                 U, LLDumpOpts, Indent);
       } else {
         OS << '\n';
         OS.indent(Indent);
         OS << formatv("error extracting location list: {0}",
-                    fmt_consume(ExpectedLL.takeError()));
+                      fmt_consume(LL.takeError()));
       }
-  };
-  if (FormValue.isFormClass(DWARFFormValue::FC_SectionOffset)) {
-    uint64_t Offset = *FormValue.getAsSectionOffset();
-    if (!U->isDWOUnit() && !U->getLocSection()->Data.empty()) {
-      DWARFDebugLoc DebugLoc;
-      DWARFDataExtractor Data(Obj, *U->getLocSection(), Ctx.isLittleEndian(),
-                              Obj.getAddressSize());
-      DumpLL(DebugLoc.parseOneLocationList(Data, &Offset));
       return;
     }
 
     bool UseLocLists = !U->isDWOUnit();
     auto Data =
         UseLocLists
             ? DWARFDataExtractor(Obj, Obj.getLoclistsSection(),
                                  Ctx.isLittleEndian(), Obj.getAddressSize())
             : DWARFDataExtractor(U->getLocSectionData(), Ctx.isLittleEndian(),
                                  Obj.getAddressSize());
 
     if (!Data.getData().empty()) {
       // 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.
-      DumpLL(DWARFDebugLoclists::parseOneLocationList(
-          Data, &Offset, UseLocLists ? U->getVersion() : 4));
+      DWARFDebugLoclists::dumpLocationList(
+          Data, &Offset, UseLocLists ? U->getVersion() : 4, OS, BaseAddr, MRI,
+          U, LLDumpOpts, Indent);
     }
+    return;
   }
+
+  FormValue.dump(OS, DumpOpts);
 }
 
 /// Dump the name encoded in the type tag.
 static void dumpTypeTagName(raw_ostream &OS, dwarf::Tag T) {
   StringRef TagStr = TagString(T);
   if (!TagStr.startswith("DW_TAG_") || !TagStr.endswith("_type"))
     return;
   OS << TagStr.substr(7, TagStr.size() - 12) << " ";

llvm/test/CodeGen/X86/debug-loclists.ll

 ; CHECK:      .debug_loclists contents:
 ; CHECK-NEXT: 0x00000000: locations list header: length = 0x00000029, version = 0x0005, addr_size = 0x08, seg_size = 0x00, offset_entry_count = 0x00000000
 
 ; Don't use startx_length if there's more than one entry, because the shared
 ; base address will be useful for both the range that does start at the start of
 ; the function, and the one that doesn't.
 
 ; CHECK-NEXT: 0x0000000c:
-; CHECK-NEXT:             DW_LLE_base_addressx(0x0000000000000000)
-; CHECK-NEXT:             DW_LLE_offset_pair  (0x0000000000000000, 0x0000000000000003)
+; CHECK-NEXT:             DW_LLE_base_addressx (0x0000000000000000)
+; CHECK-NEXT:             DW_LLE_offset_pair   (0x0000000000000000, 0x0000000000000003)
 ; CHECK-NEXT:                              => [0x0000000000000000, 0x0000000000000003): DW_OP_consts +3, DW_OP_stack_value
-; CHECK-NEXT:             DW_LLE_offset_pair  (0x0000000000000003, 0x0000000000000004)
+; CHECK-NEXT:             DW_LLE_offset_pair   (0x0000000000000003, 0x0000000000000004)
 ; CHECK-NEXT:                              => [0x0000000000000003, 0x0000000000000004): DW_OP_consts +4, DW_OP_stack_value
-; CHECK-NEXT:             DW_LLE_end_of_list  ()
+; CHECK-NEXT:             DW_LLE_end_of_list   ()
 
 ; Show that startx_length can be used when the address range starts at the start of the function.
 
 ; CHECK:      0x0000001d:
-; CHECK-NEXT:             DW_LLE_startx_length(0x0000000000000000, 0x0000000000000003)
+; CHECK-NEXT:             DW_LLE_startx_length (0x0000000000000000, 0x0000000000000003)
 ; CHECK-NEXT:                              => Addr idx 0 (w/ length 3): DW_OP_consts +5, DW_OP_stack_value
-; CHECK-NEXT:             DW_LLE_end_of_list  ()
+; CHECK-NEXT:             DW_LLE_end_of_list   ()
 
 ; And use a base address when the range doesn't start at an existing/useful
 ; address in the pool.
 
 ; CHECK:      0x00000025:
-; CHECK-NEXT:             DW_LLE_base_addressx(0x0000000000000000)
-; CHECK-NEXT:             DW_LLE_offset_pair  (0x0000000000000003, 0x0000000000000004)
+; CHECK-NEXT:             DW_LLE_base_addressx (0x0000000000000000)
+; CHECK-NEXT:             DW_LLE_offset_pair   (0x0000000000000003, 0x0000000000000004)
 ; CHECK-NEXT:                              => [0x0000000000000003, 0x0000000000000004): DW_OP_reg0 RAX
-; CHECK-NEXT:             DW_LLE_end_of_list  ()
+; CHECK-NEXT:             DW_LLE_end_of_list   ()
 
 ; Built with clang -O3 -ffunction-sections from source:
 ; 
 ; int f1(int i, int j) {
 ;   int x = 5;
 ;   int y = 3;
 ;   int r = i + j;
 ;   int undef;

llvm/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s

 # RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
 # RUN: llvm-dwarfdump %t | FileCheck %s
 
 # CHECK:      DW_AT_name        ("x0")
 # CHECK-NEXT: DW_AT_location    (0x0000000c
 # CHECK-NEXT:    [0x0000000000000000,  0x0000000000000002): DW_OP_reg5 RDI
 # CHECK-NEXT:    [0x0000000000000002,  0x0000000000000003): DW_OP_reg0 RAX)
 
 # CHECK:      DW_AT_name        ("x1")
 # CHECK-NEXT: DW_AT_location    (0xdeadbeef
-# CHECK-NEXT:    error extracting location list: unexpected end of data)
+# CHECK-NEXT:    error: unexpected end of data)
 
 # CHECK:      DW_AT_name        ("x2")
 # CHECK-NEXT: DW_AT_location    (0x00000025
-# CHECK-NEXT:    error extracting location list: unexpected end of data)
+# CHECK-NEXT:    error: unexpected end of data)
 
 
         .type   f,@function
 f:                                      # @f
 .Lfunc_begin0:
         movl    %edi, %eax
 .Ltmp0:
         retq

llvm/test/DebugInfo/X86/dwarfdump-debug-loclists.test

 # RUN: llvm-mc %s -filetype obj -triple x86_64-pc-linux -o %t.o
 # RUN: llvm-dwarfdump -v %t.o | FileCheck %s
 
 # CHECK:      .debug_info
 # CHECK:       DW_AT_name{{.*}}"stub"
 # CHECK:       DW_AT_location [DW_FORM_sec_offset]   (0x0000000c
 # CHECK-NEXT:    [0x0000000000000010, 0x0000000000000020): DW_OP_breg5 RDI+0
 # CHECK-NEXT:    [0x0000000000000530, 0x0000000000000540): DW_OP_breg6 RBP-8, DW_OP_deref
 # CHECK-NEXT:    [0x0000000000000700, 0x0000000000000710): DW_OP_breg5 RDI+0
 
 # CHECK:      .debug_loclists contents:
 # CHECK-NEXT: 0x00000000: locations list header: length = 0x0000002c, version = 0x0005, addr_size = 0x08, seg_size = 0x00, offset_entry_count = 0x00000000
 # CHECK-NEXT: 0x0000000c:
-# CHECK-NEXT:   DW_LLE_offset_pair (0x0000000000000000, 0x0000000000000010)
+# CHECK-NEXT:   DW_LLE_offset_pair  (0x0000000000000000, 0x0000000000000010)
 # CHECK-NEXT:                   => [0x0000000000000000, 0x0000000000000010): DW_OP_breg5 RDI+0
-# CHECK-NEXT:   DW_LLE_base_address(0x0000000000000500)
-# CHECK-NEXT:   DW_LLE_offset_pair (0x0000000000000030, 0x0000000000000040)
+# CHECK-NEXT:   DW_LLE_base_address (0x0000000000000500)
+# CHECK-NEXT:   DW_LLE_offset_pair  (0x0000000000000030, 0x0000000000000040)
 # CHECK-NEXT:                   => [0x0000000000000530, 0x0000000000000540): DW_OP_breg6 RBP-8, DW_OP_deref
-# CHECK-NEXT:   DW_LLE_start_length(0x0000000000000700, 0x0000000000000010)
+# CHECK-NEXT:   DW_LLE_start_length (0x0000000000000700, 0x0000000000000010)
 # CHECK-NEXT:                   => [0x0000000000000700, 0x0000000000000710): DW_OP_breg5 RDI+0
-# CHECK-NEXT:   DW_LLE_end_of_list ()
+# CHECK-NEXT:   DW_LLE_end_of_list  ()
 
 .section  .debug_str,"MS",@progbits,1
   .asciz  "stub"
 
 .section  .debug_str_offsets,"",@progbits
   .long  68
   .short  5
   .short  0

llvm/test/DebugInfo/X86/fission-ranges.ll

 ; CHECK-NEXT: DW_AT_low_pc
 ; CHECK-NEXT: DW_AT_high_pc
 ; CHECK-NEXT: DW_AT_GNU_addr_base [DW_FORM_sec_offset]                   (0x00000000)
 
 ; CHECK: .debug_info.dwo contents:
 ; CHECK:      DW_TAG_formal_parameter
 ; CHECK-NEXT:   DW_AT_const_value [DW_FORM_sdata] (1)
 ; CHECK-NEXT:   DW_AT_name {{.*}} "p")
-; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[A:0x[0-9a-z]*]]
-; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[E:0x[0-9a-z]*]]
-; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[B:0x[0-9a-z]*]]
-; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[D:0x[0-9a-z]*]]
+; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[A:0x[0-9a-z]*]]:
+; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[E:0x[0-9a-z]*]]:
+; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[B:0x[0-9a-z]*]]:
+; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[D:0x[0-9a-z]*]]:
 ; CHECK: DW_AT_ranges [DW_FORM_sec_offset]   (0x00000000
 ; CHECK-NOT: .debug_loc contents:
 ; CHECK-NOT: Beginning address offset
 ; CHECK: .debug_loc.dwo contents:
 
 ; Don't assume these locations are entirely correct - feel free to update them
 ; if they've changed due to a bugfix, change in register allocation, etc.
 
 ; CHECK:      [[A]]:
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000002, 0x0000000f)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000002, 0x0000000f)
 ; CHECK-NEXT:                    => Addr idx 2 (w/ length 15): DW_OP_consts +0, DW_OP_stack_value
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000003, 0x0000000f)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000003, 0x0000000f)
 ; CHECK-NEXT:                    => Addr idx 3 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000004, 0x00000012)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000004, 0x00000012)
 ; CHECK-NEXT:                    => Addr idx 4 (w/ length 18): DW_OP_breg7 RSP-8
-; CHECK-NEXT:   DW_LLE_end_of_list  ()
+; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:      [[E]]:
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000005, 0x00000009)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000005, 0x00000009)
 ; CHECK-NEXT:                    => Addr idx 5 (w/ length 9): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000006, 0x00000062)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000006, 0x00000062)
 ; CHECK-NEXT:                    => Addr idx 6 (w/ length 98): DW_OP_breg7 RSP-44
-; CHECK-NEXT:   DW_LLE_end_of_list  ()
+; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:      [[B]]:
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000007, 0x0000000f)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000007, 0x0000000f)
 ; CHECK-NEXT:                    => Addr idx 7 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000008, 0x00000042)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000008, 0x00000042)
 ; CHECK-NEXT:                    => Addr idx 8 (w/ length 66): DW_OP_breg7 RSP-32
-; CHECK-NEXT:   DW_LLE_end_of_list  ()
+; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:      [[D]]:
-; CHECK-NEXT:   DW_LLE_startx_length(0x00000009, 0x0000000f)
+; CHECK-NEXT:   DW_LLE_startx_length (0x00000009, 0x0000000f)
 ; CHECK-NEXT:                    => Addr idx 9 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length(0x0000000a, 0x0000002a)
+; CHECK-NEXT:   DW_LLE_startx_length (0x0000000a, 0x0000002a)
 ; CHECK-NEXT:                    => Addr idx 10 (w/ length 42): DW_OP_breg7 RSP-20
-; CHECK-NEXT:   DW_LLE_end_of_list  ()
+; CHECK-NEXT:   DW_LLE_end_of_list   ()
 
 ; Make sure we don't produce any relocations in any .dwo section (though in particular, debug_info.dwo)
 ; HDR-NOT: .rela.{{.*}}.dwo
 
 ; Make sure we have enough stuff in the debug_addr to cover the address indexes
 ; (10 is the last index in debug_loc.dwo, making 11 entries of 8 bytes each,
 ; 11 * 8 == 88 base 10 == 58 base 16)
 

llvm/test/DebugInfo/X86/stack-value-piece.ll

 ;   F r = {f, 0};
 ;   return r;
 ; }
 
 ; CHECK: .debug_info contents:
 ; CHECK: DW_TAG_subprogram
 ; CHECK:   DW_AT_name {{.*}} "i"
 ; CHECK:   DW_TAG_variable
-; CHECK-NEXT:   DW_AT_location {{.*}} ([[I:.*]]
+; CHECK-NEXT:   DW_AT_location {{.*}} ([[I:0x[0-9a-f]+]]
 ; CHECK-NEXT:     [{{.*}}, {{.*}}): DW_OP_reg5 RDI, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
 ; CHECK-NEXT:   DW_AT_name {{.*}} "r"
 ;
 ; CHECK: DW_TAG_subprogram
 ; CHECK:   DW_AT_name {{.*}} "f"
 ; CHECK:   DW_TAG_variable
-; CHECK-NEXT:   DW_AT_location {{.*}} ([[F:.*]]
+; CHECK-NEXT:   DW_AT_location {{.*}} ([[F:0x[0-9a-f]+]]
 ; CHECK-NEXT:     [{{.*}}, {{.*}}): DW_OP_reg17 XMM0, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4
 ; CHECK-NEXT:     [{{.*}}, {{.*}}): DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
 ; CHECK-NEXT:   DW_AT_name {{.*}} "r"
 ;
 ; CHECK: .debug_loc contents:
 ; CHECK:      [[I]]:
 ; CHECK-NEXT:   [{{.*}}, {{.*}}): DW_OP_reg5 RDI, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4
 ; CHECK:      [[F]]:

llvm/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 = 0x0000000e, version = 0x0005, addr_size = 0x08, seg_size = 0x00, offset_entry_count = 0x00000000
 # CHECK-NEXT:    0x0000000c:
-# CHECK-NEXT:                DW_LLE_startx_length(0x0000000000000001, 0x0000000000000010)
+# CHECK-NEXT:                DW_LLE_startx_length (0x0000000000000001, 0x0000000000000010)
 # CHECK-NEXT:                                 => Addr idx 1 (w/ length 16): DW_OP_reg5 RDI
-# CHECK-NEXT:                DW_LLE_end_of_list  ()
+# CHECK-NEXT:                DW_LLE_end_of_list   ()
 
 .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
  .byte 1          # Loc expr size
  .byte 85         # DW_OP_reg5
  .byte 0          # DW_LLE_end_of_list
 .Ldebug_loclist_table_end0: