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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp:24
   uint64_t A = getUnsigned(Off, Size, Err);
   if (!E)
     return A;
----------------
labath wrote:
> 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).
Thanks for the suggestion. (I was ready to abandon this but it looks like there is value with this change)

Do you have suggestions how I should test this change? No call site passes in the Error parameter when calling `getRelocatedAddress`. I am not sure what is the simplest way to let the various .debug* parser stop parsing immediately.

The test should probably use yaml2obj.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67343