Use DataExtractor to decode SLEB128 in android_relas.

[rahmanl]

Summary

A simple refactoring patch which let us use DataExtractor::getSLEB128 rather than using a lambda function.

[rahmanl]

Remove some of the error handling code.

[rahmanl]

Make the error handling code consistent with the old code.

[probinson]

llvm/lib/Object/ELF.cpp

13

Can you remove the LEB128.h include?

[jhenderson]

In principle, looks fine, but two pre-merge bots are complaining about an unhandled Error somewhere.

[rahmanl]

Add an extra error check to solve the buildbot failure.

[rahmanl]

Thanks @jhenderson. I added one more check after uint64_t NumRelocsInGroup = Data.getSLEB128(Cur); to solve the issue.

[jhenderson]

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.

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.

llvm/lib/Object/ELF.cpp

383

Add a comment saying what the 4 here represents, i.e. something like DataExtractor::Cursor Cur(/*Offset=*/4);

[grimar]

A minor nit from me, the rest looks fine.

llvm/lib/Object/ELF.cpp

435

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.

[rahmanl]

In 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.

llvm/lib/Object/ELF.cpp

435

Great suggestion. Thanks.

[rahmanl]

Address two reviewer comments.

[rahmanl]

rebase.

[MaskRay]

LG

[rahmanl]

@jhenderson Is this ready to land?

[jhenderson]

In D95158#2524564, @rahmanl wrote:

@jhenderson Is this ready to land?

Sorry, I got caught up with things yesterday and didn't get to all the review requests I had to field.

I guess the test case I'm thinking of is showing that the malformed group reloc count value means we don't get the "relocation group unexpectedly large" error. I don't feel strongly enough about this though to push for more testing if you don't want to add it.

[rahmanl]

In D95158#2524766, @jhenderson wrote:

In D95158#2524564, @rahmanl wrote:

@jhenderson Is this ready to land?

Sorry, I got caught up with things yesterday and didn't get to all the review requests I had to field.

I guess the test case I'm thinking of is showing that the malformed group reloc count value means we don't get the "relocation group unexpectedly large" error. I don't feel strongly enough about this though to push for more testing if you don't want to add it.

Thanks for the response. I see your point now. But the "relocation group unexpectedly large" error has been tested by asm5.s in the same test file above. So I think we should be good to land.

[rahmanl]

I am surprised this patch hasn't been automatically updated to show the committed status after I pushed it (https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b). Maybe I should've used arc land instead of git push?

[grimar]

In D95158#2529509, @rahmanl wrote:

I am surprised this patch hasn't been automatically updated to show the committed status after I pushed it (https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b). Maybe I should've used arc land instead of git push?

Probably because you've used "Differential Revision". The latter word should be "revision"

[jhenderson]

In D95158#2530091, @grimar wrote:

In D95158#2529509, @rahmanl wrote:

I am surprised this patch hasn't been automatically updated to show the committed status after I pushed it (https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b). Maybe I should've used arc land instead of git push?

Probably because you've used "Differential Revision". The latter word should be "revision"

No, it definitely should be Revision (upper-case). I always use git push, and my last commit worked fine - see rGb37a349ff2442e73ceafeee982afb430359e08b1. Your format looks the same as mine, so either Phabricator had a fit and just failed to link the two up this time, or something else is the matter. @mehdi_amini, any ideas?

[jhenderson]

Oh, don't forget to manually close and add the related commit yourself.

[grimar]

In D95158#2530153, @jhenderson wrote:

In D95158#2530091, @grimar wrote:

In D95158#2529509, @rahmanl wrote:

I am surprised this patch hasn't been automatically updated to show the committed status after I pushed it (https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b). Maybe I should've used arc land instead of git push?

Probably because you've used "Differential Revision". The latter word should be "revision"

No, it definitely should be Revision (upper-case).

Seems there is no difference what to use then. I believe I always used lower-case. E.g rG029644ee510792120f1c0daf32989b401d4ce871.

[rahmanl]

Closed [manually] by commit https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b

[rahmanl]

In D95158#2530158, @grimar wrote:

In D95158#2530153, @jhenderson wrote:

In D95158#2530091, @grimar wrote:

In D95158#2529509, @rahmanl wrote:

I am surprised this patch hasn't been automatically updated to show the committed status after I pushed it (https://github.com/llvm/llvm-project/commit/3ca502a7d60787abe14db1fa541950ff79c7585b). Maybe I should've used arc land instead of git push?

Probably because you've used "Differential Revision". The latter word should be "revision"

No, it definitely should be Revision (upper-case).

Seems there is no difference what to use then. I believe I always used lower-case. E.g rG029644ee510792120f1c0daf32989b401d4ce871.

Thanks @grimar and @jhenderson.
I think the 'differential revision' line was added automatically when I pushed.

llvm/lib/Object/ELF.cpp

 //===- ELF.cpp - ELF object file implementation ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/ELF.h"
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/LEB128.h"
 
 using namespace llvm;
 using namespace object;
 
 #define STRINGIFY_ENUM_CASE(ns, name)                                          \
   case ns::name:                                                               \
     return #name;
 

 template <class ELFT>
 Expected<std::vector<typename ELFT::Rela>>
 ELFFile<ELFT>::android_relas(const Elf_Shdr &Sec) const {
   // This function reads relocations in Android's packed relocation format,
   // which is based on SLEB128 and delta encoding.
   Expected<ArrayRef<uint8_t>> ContentsOrErr = getSectionContents(Sec);
   if (!ContentsOrErr)
     return ContentsOrErr.takeError();
-  const uint8_t *Cur = ContentsOrErr->begin();
-  const uint8_t *End = ContentsOrErr->end();
-  if (ContentsOrErr->size() < 4 || Cur[0] != 'A' || Cur[1] != 'P' ||
-      Cur[2] != 'S' || Cur[3] != '2')
+  ArrayRef<uint8_t> Content = *ContentsOrErr;
+  if (Content.size() < 4 || Content[0] != 'A' || Content[1] != 'P' ||
+      Content[2] != 'S' || Content[3] != '2')
     return createError("invalid packed relocation header");
-  Cur += 4;
+  DataExtractor Data(Content, isLE(), ELFT::Is64Bits ? 8 : 4);
+  DataExtractor::Cursor Cur(4);
 
-  const char *ErrStr = nullptr;
-  auto ReadSLEB = [&]() -> int64_t {
-    if (ErrStr)
-      return 0;
-    unsigned Len;
-    int64_t Result = decodeSLEB128(Cur, &Len, End, &ErrStr);
-    Cur += Len;
-    return Result;
-  };
-
-  uint64_t NumRelocs = ReadSLEB();
-  uint64_t Offset = ReadSLEB();
+  uint64_t NumRelocs = Data.getSLEB128(Cur);
+  uint64_t Offset = Data.getSLEB128(Cur);
+  if (!Cur)
+    return std::move(Cur.takeError());
   uint64_t Addend = 0;
 
-  if (ErrStr)
-    return createError(ErrStr);
-
   std::vector<Elf_Rela> Relocs;
   Relocs.reserve(NumRelocs);
   while (NumRelocs) {
-    uint64_t NumRelocsInGroup = ReadSLEB();
+    uint64_t NumRelocsInGroup = Data.getSLEB128(Cur);
+    if (!Cur)
+      return std::move(Cur.takeError());
     if (NumRelocsInGroup > NumRelocs)
       return createError("relocation group unexpectedly large");
     NumRelocs -= NumRelocsInGroup;
 
-    uint64_t GroupFlags = ReadSLEB();
+    uint64_t GroupFlags = Data.getSLEB128(Cur);
+    if (!Cur)
+      return std::move(Cur.takeError());
     bool GroupedByInfo = GroupFlags & ELF::RELOCATION_GROUPED_BY_INFO_FLAG;
     bool GroupedByOffsetDelta = GroupFlags & ELF::RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG;
     bool GroupedByAddend = GroupFlags & ELF::RELOCATION_GROUPED_BY_ADDEND_FLAG;
     bool GroupHasAddend = GroupFlags & ELF::RELOCATION_GROUP_HAS_ADDEND_FLAG;
 
     uint64_t GroupOffsetDelta;
     if (GroupedByOffsetDelta)
-      GroupOffsetDelta = ReadSLEB();
+      GroupOffsetDelta = Data.getSLEB128(Cur);
 
     uint64_t GroupRInfo;
     if (GroupedByInfo)
-      GroupRInfo = ReadSLEB();
+      GroupRInfo = Data.getSLEB128(Cur);
 
     if (GroupedByAddend && GroupHasAddend)
-      Addend += ReadSLEB();
+      Addend += Data.getSLEB128(Cur);
+    if (!Cur)
+      return std::move(Cur.takeError());
 
     if (!GroupHasAddend)
       Addend = 0;
 
     for (uint64_t I = 0; I != NumRelocsInGroup; ++I) {
       Elf_Rela R;
-      Offset += GroupedByOffsetDelta ? GroupOffsetDelta : ReadSLEB();
+      Offset += GroupedByOffsetDelta ? GroupOffsetDelta : Data.getSLEB128(Cur);
       R.r_offset = Offset;
-      R.r_info = GroupedByInfo ? GroupRInfo : ReadSLEB();
+      R.r_info = GroupedByInfo ? GroupRInfo : Data.getSLEB128(Cur);
       if (GroupHasAddend && !GroupedByAddend)
-        Addend += ReadSLEB();
+        Addend += Data.getSLEB128(Cur);
       R.r_addend = Addend;
       Relocs.push_back(R);
-
-      if (ErrStr)
-        return createError(ErrStr);
+      if (!Cur)
+        return std::move(Cur.takeError());
     }
-
-    if (ErrStr)
-      return createError(ErrStr);
   }
 
   return Relocs;
 }
 
 template <class ELFT>
 std::string ELFFile<ELFT>::getDynamicTagAsString(unsigned Arch,
                                                  uint64_t Type) const {

llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s

 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm2.s -o %t2.o
 # RUN: llvm-readobj --relocations %t2.o 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR-PAST-END
 # RUN: llvm-readelf --relocations %t2.o 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR-PAST-END
 
 #--- asm2.s
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"
 
-# ERR-PAST-END: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: malformed sleb128, extends past end
+# ERR-PAST-END: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: unable to decode LEB128 at offset 0x{{([[:xdigit:]]{8})}}: malformed sleb128, extends past end
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm3.s -o %t3.o
 # RUN: llvm-readobj --relocations %t3.o 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=ERR-PAST-END
 # RUN: llvm-readelf --relocations %t3.o 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=ERR-PAST-END
 
 #--- asm3.s
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"