[llvm-readobj] Little clean up inside `parseDynamicTable`

[Higuoxing]

Summary

This anoymous function actually has same logic with Obj->toMappedAddr.

Besides, I have a question on resolving illegal value. gnu-readelf, gnu-objdump and llvm-objdump could parse the test file 'test/tools/llvm-objdump/Inputs/private-headers-x86_64.elf', but llvm-readobj will fail when parse DT_RELR segment. Because, the value is 0x87654321 which is illegal. So, shall we do this clean up rather then remove the checking statements inside anoymous function?

if (Delta >= Phdr.p_filesz)
    return createError("Virtual address is not in any segment");

[jhenderson]

LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).

As for whether or not we should error in this case, the problem is that things like the Hash table are looked up using this function, and then an attempt may be made to dump the hash table from the loaded address. Potentially, this could lead to trying to access invalid memory, which is obviously bad. So perhaps before making any decision, you should look up what the implications would be of removing the error?

[Higuoxing]

In D55329#1322718, @jhenderson wrote:

LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).

As for whether or not we should error in this case, the problem is that things like the Hash table are looked up using this function, and then an attempt may be made to dump the hash table from the loaded address. Potentially, this could lead to trying to access invalid memory, which is obviously bad. So perhaps before making any decision, you should look up what the implications would be of removing the error?

Thanks for reviewing! I will look into it : )

[Higuoxing]

Rebase

[sidneym]

FWIW: The check you mention in ELF.cpp is triggered by the following test script. This causes llvm-readelf to generate an unexpected error message, LLVM ERROR: Virtual address is not in any segment

cat > 1.c << \!
int foo = 10;
int main() { return foo; }
!

cat > script.t << \!
PHDRS {
  CODE_RO PT_LOAD;
  DATA PT_LOAD;
  GOT PT_LOAD;
  DYN PT_LOAD;
  DYNAMIC PT_DYNAMIC;
  RELADYN PT_LOAD;
}
SECTIONS {
  .text : { *(.text) } :CODE_RO
  .data : { *(.data) } :DATA
  .dynamic : { *(.dynamic) }: DYN : DYNAMIC
  .rela.dyn : {*(.rel.dyn)} : RELADYN
}
!

gcc -c 1.c -fPIC
ld -shared 1.o -T script.t -Bsymbolic -pie -soname foo -o foo
echo llvm-readelf:
llvm-readelf -d foo
echo readelf:
readelf -d foo

[Higuoxing]

In D55329#1328925, @sidneym wrote:

FWIW: The check you mention in ELF.cpp is triggered by the following test script. This causes llvm-readelf to generate an unexpected error message, LLVM ERROR: Virtual address is not in any segment

cat > 1.c << \!
int foo = 10;
int main() { return foo; }
!

cat > script.t << \!
PHDRS {
  CODE_RO PT_LOAD;
  DATA PT_LOAD;
  GOT PT_LOAD;
  DYN PT_LOAD;
  DYNAMIC PT_DYNAMIC;
  RELADYN PT_LOAD;
}
SECTIONS {
  .text : { *(.text) } :CODE_RO
  .data : { *(.data) } :DATA
  .dynamic : { *(.dynamic) }: DYN : DYNAMIC
  .rela.dyn : {*(.rel.dyn)} : RELADYN
}
!

gcc -c 1.c -fPIC
ld -shared 1.o -T script.t -Bsymbolic -pie -soname foo -o foo
echo llvm-readelf:
llvm-readelf -d foo
echo readelf:
readelf -d foo

Thank you @sidneym, it's very helpful!

I am considering suppress the failure in llvm-readobj and just give a warning. I think this tool should give us enough information on binaries and should not crash because of an invalid value, right? But I still have no idea by now ... I don't want break the ELF.cpp internal API ...

tools/llvm-readobj/ELFDumper.cpp

ELFDumper<ELFT>::ELFDumper(const ELFFile<ELFT> *Obj, ScopedPrinter &Writer)
   else
     ELFDumperStyle.reset(new LLVMStyle<ELFT>(Writer, this));
 }
 
 template <typename ELFT>
 void ELFDumper<ELFT>::parseDynamicTable(
     ArrayRef<const Elf_Phdr *> LoadSegments) {
   auto toMappedAddr = [&](uint64_t VAddr) -> const uint8_t * {
-    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())
-      report_fatal_error("Virtual address is not in any segment");
-    --I;
-    const Elf_Phdr &Phdr = **I;
-    uint64_t Delta = VAddr - Phdr.p_vaddr;
-    if (Delta >= Phdr.p_filesz)
-      report_fatal_error("Virtual address is not in any segment");
-    return Obj->base() + Phdr.p_offset + Delta;
+    auto MappedAddrOrError = Obj->toMappedAddr(VAddr);
+    if (!MappedAddrOrError)
+      report_fatal_error(MappedAddrOrError.takeError());
+    return MappedAddrOrError.get();
   };
 
   uint64_t SONameOffset = 0;
   const char *StringTableBegin = nullptr;
   uint64_t StringTableSize = 0;
   for (const Elf_Dyn &Dyn : dynamic_table()) {
     switch (Dyn.d_tag) {
     case ELF::DT_HASH: