[PATCH] D95158: Use DataExtractor to decode SLEB128 in android_relas.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked 2 inline comments as done.
rahmanl added a comment.

In D95158#2518980 <https://reviews.llvm.org/D95158#2518980>, @jhenderson wrote:

> So, if the previous build bot failure was due to an unhandled `Error`, that means that this error path doesn't have any testing. Basically, there needs to be a test for each individual `getSLEB128` call, in case the section happens to be corrupted at that specific point (e.g. a truncated SLEB or one that goes outside the uint64_t range). Given that this is in the Object library, it would also probably make most sense for those tests to be gtest unit tests rather than lit stuff.

I agree that the testing is not extensive. However, I think as long as we make sure the Cursor error is eventually taken (by `takeError`), there won't be any unhandled error problem. The previous diff posed the problem that the following return from the function (at line 396) could potentially left the Cursor error unhandled.

  if (NumRelocsInGroup > NumRelocs)
     return createError("relocation group unexpectedly large");

I am trying to say that we only need to guard the function returns (which is already done). So I don't have a strong opinion on making the tests any better.

> If I'm following things correctly, I don't think you've made the testing situation any worse, so if you'd prefer to defer that to a separate change, that's fine by me.





================
Comment at: llvm/lib/Object/ELF.cpp:431
+      if (!Cur)
+        return std::move(Cur.takeError());
     }
----------------
grimar wrote:
> Looks like you can remove these 2 lines and do the following?
> 
> ```
> for (uint64_t I = 0; Cur && I != NumRelocsInGroup; ++I) {
> ```
> 
> It is a bit shorter, anyways the `Cur` is checked right below.
Great suggestion. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95158