[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

David Blaikie via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Jun 20 12:48:43 PDT 2019

dblaikie added inline comments.

Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:27
+template <typename... Ts>
+static llvm::Error createError(const char *Fmt, Ts &&... Vals) {
+  return createStringError(inconvertibleErrorCode(), Fmt, Vals...);
I guess "Ts &&... Vals" should be "const Ts &... Vals" since they're taken by const ref by createStringError anyway - no need for the fancy &&.

Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:31
+static llvm::Error createOverflowError(const char *Section) {
+  return createError("location list overflows the %s section", Section);
Should this be StringRef rather than const char*?

Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:171
+    case dwarf::DW_LLE_offset_pair:
       E.Value0 = Data.getULEB128(Offset);
+      break;
Looks to me like getULEB128 doesn't quite have the right error handling, if I'm reading it correctly:

  unsigned shift = 0;
  uint32_t offset = *offset_ptr;
  uint8_t byte = 0;

  while (isValidOffset(offset)) {
    byte = Data[offset++];
    result |= uint64_t(byte & 0x7f) << shift;
    shift += 7;
    if ((byte & 0x80) == 0)

  *offset_ptr = offset;
  return result;

I /imagine/ it shouldn't update offset_ptr if it breaks out of the loop via !isValidOffset, rather than via the break?

More broadly, I wonder if we should consider a more convenient way to do error handling here - since it's a bit unfortunate that you've had to split the logic for parsing these things across two switch statements - makes it a bit hard to follow what shape each LLE entry has, since it's spread out like this.

Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220
-  return LL;
+  llvm_unreachable("Exit from function inside while loop!");
Given the loop condition is "while (true)" this unreachable seems a bit unnecessary (& the function has non-void return, so if there was a path that got through the loop I imagine the compiler would warn us about that?)

Or is this working around a compiler that warns here despite the lack of any path out of the loop?




More information about the lldb-commits mailing list