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

[David Blaikie via Phabricator via lldb-commits]

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)
      break;
  }

  *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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591