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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:31
+
+static llvm::Error createOverflowError(const char *Section) {
+  return createError("location list overflows the %s section", Section);
----------------
dblaikie wrote:
> Should this be StringRef rather than const char*?
createStringError uses a printf format string. Taking a StringRef would mean I'd have to add `.str().c_str()` blurb.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:171
+    case dwarf::DW_LLE_offset_pair:
       E.Value0 = Data.getULEB128(Offset);
+      break;
----------------
dblaikie wrote:
> 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.
Nice catch about the getULEB function. I'll create a separate patch for that.

Overall, I'm not really happy about how the error handling is implemented here. The DataExtractor functions seem to be really good at making sure you don't crash while using them, but they also make it incredibly hard to check the result for errors. We could change them to return an Optional<T> or something, but that would make using them a lot more verbose.

It sounds like me like it may be best to have some thing similar to what std::istream has. I.e., have an object which encapsulates three things:
- the data to parse
- current offset in that data
- an error flag
This would mean one can still call GetXXX functions in sequence without additional error checking. However, at a suitable point in time (e.g., after parsing a single record/DIE/...), one can have a peek at the error flag to verify that the data he got is actually valid.

WDYT?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220
   }
-  return LL;
+  llvm_unreachable("Exit from function inside while loop!");
 }
----------------
probinson wrote:
> dblaikie wrote:
> > 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?
> I have had to add llvm_unreachable before in this kind of situation, IIRC, which is why I suggested it.  Might not be necessary, if all 3 supported toolchains are smart enough nowadays.
The debug_loc parser already uses this pattern without the terminating llvm_unreachable so I'd say we can assume the current compilers are fine with that..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591