[llvm-readelf/obj] - Handle out-of-order PT_LOADs better.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=45698.

Specification says that
"Loadable segment entries in the program header table appear
in ascending order, sorted on the p_vaddr member."

Our toMappedAddr() relies on this condition. This patch
adds a warning when the sorting order of loadable segments is wrong.
In this case we force segments sorting and that allows
toMappedAddr() to work as expected.

[jhenderson]

From the description:
"Out toMappedAddr()" -> "Our toMappedAddr()"

llvm/lib/Object/ELF.cpp

580–586

I know this is what the ELF specification says, but I wonder whether it would be almost as easy to sort the segments here (maybe in addition to a warning, specified via a callback probably, if they are unsorted), rather than just giving up? That would be somewhat more useful than just warning and not doing anything.

Also, this is library code - is this code used elsewhere? It might be more appropriate for the test case to be in the Object tests.

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

419

[grimar]

• Addressed review comments.

llvm/lib/Object/ELF.cpp

580–586

Done.

[grimar]

llvm/lib/Object/ELF.cpp

580–586

Also, this is library code - is this code used elsewhere? It might be more appropriate for the test case to be in the Object tests.

Yes, it is used in lib\InterfaceStub. I've added the unit test.

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

I think if DataOrErr is false, we need to somehow handle the error, right? Actually maybe this should just be ASSERT_TRUE?

[grimar]

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

I can't use ASSERT_TRUE here (or ASSERT_THAT_EXPECTED). Its intention is to return instantly,
and so it calls return internally I think. And so I can't use it in lambda:

Error	C2440	'return': cannot convert from 'void' to 'const uint8_t *'	ObjectTests	D:\Work3\LLVM\llvm-project\llvm\unittests\Object\ELFObjectFileTest.cpp	377

I think if DataOrErr is false, we need to somehow handle the error, right?

I don't think it is very important here? We expect toMappedAddr call to succeed here,
this is what test is wrapped around. When something in a unit test goes wrong,
we can probably just report and fail, no matter how.

[grimar]

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

I can consume an error and return nullptr right after EXPECT_TRUE((bool)DataOrErr) probably.
It should be a bit cleaner. Should I?

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

If you fail to consume the error in one form or other, this test will crash if the DataOrErr check fails, either from the unchecked error or the final dereference of DataOrErr at the end of the function, rather than producing a useful "test failed" type message. I don't think that's desirable. You could always try reporting the error via the << syntax if the check fails before returning. I can't remember the exact syntax, but I believe it's something like EXPECT_TRUE((bool)DataOrErr) << toString(DataOrErr.takeError()); That being said, shouldn't this be EXPECT_THAT_EXPECTED(DataOrErr, Succeeded());? I'm guessing that consumes the error safely (you'll still need to be careful about the dereference).

[grimar]

• Addressed review comment.

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

I've used EXPECT_THAT_EXPECTED. Yes, it calls handleAllErrors internally.

[grimar]

• Fixed an issue with the unit test.

llvm/unittests/Object/ELFObjectFileTest.cpp

377 ↗

(On Diff #310504)

I've found that EXPECT_THAT_EXPECTED doesn't help much here.

The problem is in the line below:

return !DataOrErr ? nullptr : *DataOrErr;

EXPECT_THAT_EXPECTED takes the error from DataOrErr,
but the unchecked bit is then set again by the operator bool:

  explicit operator bool() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
    Unchecked = HasError;
#endif
    return !HasError;
  }

I think we should handle an error in a straighforward way here.

[jhenderson]

llvm/unittests/Object/ELFObjectFileTest.cpp

391 ↗

(On Diff #311556)

Perhaps you could add a single ASSERT_TRUE(Data) (or similar)? You could then make this and the equivalent line below something like EXPECT_EQ(Data[0], 0x11); which would give a nicer message.

[grimar]

• Addressed review comment.

llvm/unittests/Object/ELFObjectFileTest.cpp

391 ↗

(On Diff #311556)

Done.

[jhenderson]

LGTM.

llvm/lib/Object/ELF.cpp

if (!ProgramHeadersOrError)
     return ProgramHeadersOrError.takeError();
 
   llvm::SmallVector<Elf_Phdr *, 4> LoadSegments;
 
   for (const Elf_Phdr &Phdr : *ProgramHeadersOrError)
     if (Phdr.p_type == ELF::PT_LOAD)
       LoadSegments.push_back(const_cast<Elf_Phdr *>(&Phdr));
 
+  if (!llvm::is_sorted(LoadSegments, [](const Elf_Phdr_Impl<ELFT> *A,
+                                        const Elf_Phdr_Impl<ELFT> *B) {
+        return A->p_vaddr < B->p_vaddr;
+      }))
+    return createError(
+        "unable to map address 0x" + Twine::utohexstr(VAddr) +
+        " to a segment: loadable segments must be in ascending address order");
+
   const Elf_Phdr *const *I =
       std::upper_bound(LoadSegments.begin(), LoadSegments.end(), VAddr,
                        [](uint64_t VAddr, const Elf_Phdr_Impl<ELFT> *Phdr) {
                          return VAddr < Phdr->p_vaddr;
                        });
 
   if (I == LoadSegments.begin())
     return createError("virtual address is not in any segment: 0x" +

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

 # PAST-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
+
+## Check that we report a warning when we try to map an address, but loadable
+## segments appear unsorted on the p_vaddr member.
+
+# RUN: yaml2obj %s --docnum=7 -o %t10
+# RUN: llvm-readobj --dynamic-table %t10 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10 --implicit-check-not=warning: --check-prefixes=OUT-OF-ORDER,OUT-OF-ORDER-LLVM
+# RUN: llvm-readelf --dynamic-table %t10 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10 --implicit-check-not=warning: --check-prefixes=OUT-OF-ORDER,OUT-OF-ORDER-GNU
+
+# OUT-OF-ORDER:           warning: '[[FILE]]': unable to parse DT_STRTAB: unable to map address 0x1000 to a segment: loadable segments must be in ascending address order
+
+# OUT-OF-ORDER-LLVM:      DynamicSection [ (2 entries)
+# OUT-OF-ORDER-LLVM-NEXT:   Tag                Type   Name/Value
+# OUT-OF-ORDER-LLVM-NEXT:   0x0000000000000005 STRTAB 0x1000
+# OUT-OF-ORDER-LLVM-NEXT:   0x0000000000000000 NULL   0x0
+# OUT-OF-ORDER-LLVM-NEXT: ]
+
+# OUT-OF-ORDER-GNU:       Dynamic section at offset 0xe9 contains 2 entries:
+# OUT-OF-ORDER-GNU-NEXT:   Tag                Type     Name/Value
+# OUT-OF-ORDER-GNU-NEXT:   0x0000000000000005 (STRTAB) 0x1000
+# OUT-OF-ORDER-GNU-NEXT:   0x0000000000000000 (NULL)   0x0
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Address: 0x1000
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Address: 0x1010
+    Entries:
+      - Tag:   DT_STRTAB
+        Value: 0x1000
+      - Tag:   DT_NULL
+        Value: 0
+Symbols: []
+ProgramHeaders:
+  - Type:     PT_LOAD
+    VAddr:    0x1010
+    FirstSec: .dynamic
+    LastSec:  .dynamic
+  - Type:     PT_LOAD
+    VAddr:    0x1000
+    FirstSec: .dynstr
+    LastSec:  .dynstr
+  - Type:     PT_DYNAMIC
+    VAddr:    0x1010
+    FirstSec: .dynamic
+    LastSec:  .dynamic