[PATCH] D67343: [DebugInfo] Change object::RelocationResolver to return Expected<uint64_t>

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I believe error handling is important. And even more important is testing of that error handling -- as I'm trying to use llvm dwarf parser in lldb I keep finding new ways to crash it with invalid input.

In D63713 <https://reviews.llvm.org/D63713>, we (mainly I, I guess) tried to make checking for errors less verbose by making it possible to check for DataExtractor errors in bulk via the "Cursor" class. This is the first real test of that API, and I am afraid I have to say it's not looking too good. It (I hope) made the usage of that class easier, but it requires one to be careful when writing the code "on the inside". I explain more in the inline comment.

Since you're the first person after me who has to edit this code, I'd be very interested in any feedback you have about this.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp:24
   uint64_t A = getUnsigned(Off, Size, Err);
   if (!E)
     return A;
----------------
The tricky part here is that you cannot assume that the Error argument is always set, and you need to make sure to avoid tripping any Error assertions while making sure that the user still needs to check the error object. I was able to get away without doing any of this in this function, because the only way it could fail is through the getUnsigned function, which already does this tiptoe-ing. However, as you're introducing additional failure points here, you'll need to do that here as well.
The right incantation for that would be to insert:
```
ErrorAsOutParameter ErrAsOut(Err);
if (Err && *Err)
  return A;
```
here and then set the error object via `if (Err) *Err = R.takeError() else consumeError(R.takeError())` below.

This means that the error will be ignored in some cases, but that's the best we can to if the user has chosen not to receive errors (which has been the only way to use the data extractor until very recently).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67343