[llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.

[grimar]

Summary

It is now possible after D61937 was landed and was discussed
in it's review comments. It is not consistent with GNU, which
does not output .dynamic section content in this case for
no visible reason.

Should it be only done for LLVM style output, i.e. for llvm-readobj,
but not for GNU (llvm-readelf)?

[jhenderson]

This is an odd situation anyway (unless you know of a concrete example), so I don't think any users are likely to hit it either way. I don't think we need to follow GNU's behaviour here as a result. We should just do what makes the most sense (and GNU's behaviour doesn't make much sense to me). It would be worth getting somebody else's opinion on this too.

tools/llvm-readobj/ELFDumper.cpp

1363–1364

might want -> will
the dynamic section found -> the found dynamic section

I don't think you need the last sentence.

[grimar]

• Updated comments.

[jhenderson]

LGTM, but you may want to wait for others to comment too.

[grimar]

In D62179#1511693, @jhenderson wrote:

LGTM, but you may want to wait for others to comment too.

Yep, I'll hold it for a while.

[rupprecht]

In D62179#1510386, @jhenderson wrote:

This is an odd situation anyway (unless you know of a concrete example), so I don't think any users are likely to hit it either way. I don't think we need to follow GNU's behaviour here as a result. We should just do what makes the most sense (and GNU's behaviour doesn't make much sense to me). It would be worth getting somebody else's opinion on this too.

Same here... this patch seems fine, someone would need a use case for:

• Having dynamic sections
• Not having PT_HEADERs
• Needing llvm-readelf to be quiet about it

Which seems unlikely, but maybe wait a day or two.

[grimar]

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497

I looked on the empty-tls.test and the problem is that it contains only TLS prorgam header,
and the following call fails:

case ELF::DT_SYMTAB:
   DynSymRegion.Addr = toMappedAddr(Dyn.getPtr());
..

I.e. since there is no segment, covering the address of dynamic symbol table,
it fails to get its data properly.

We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

[rupprecht]

In D62179#1515750, @grimar wrote:

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497
...
We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

I've seen this same error internally when switching from GNU readelf to llvm-readelf and reading core files with it, and I'm interested in fixing that. GNU readelf does what you mention -- prints a warning and moves on. I think that's useful; we should be able to use tools to read partial information from a corrupt/incomplete file.

[grimar]

In D62179#1516667, @rupprecht wrote:

In D62179#1515750, @grimar wrote:

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497
...
We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

I've seen this same error internally when switching from GNU readelf to llvm-readelf and reading core files with it, and I'm interested in fixing that. GNU readelf does what you mention -- prints a warning and moves on. I think that's useful; we should be able to use tools to read partial information from a corrupt/incomplete file.

OK. I'll resurrect this one and take a look.

[grimar]

• Updated implementation and test cases in according to latest discussion comments.

(now llvm-readelf bypass the "Virtual address is not in any segment" error and will try to continue parsing the object).

[rupprecht]

Thanks for following up with this! Just one optional suggestion:

tools/llvm-readobj/ELFDumper.cpp

1468

Instead of constants like "DT_HASH" for all these changes, can you pass in Dyn.d_tag instead and use getTypeString() within toMappedAddr to print the string? (Passing Obj->getHeader()->e_machine by lambda capture). That will keep all these calls identical.

[grimar]

tools/llvm-readobj/ELFDumper.cpp

1468

Oh, cool, I did not know we have something like getTypeString() here (or perhaps forgot about that) , thanks!

[jankratochvil]

There is a regression - after this patch - at least after its later commit:

commit 8ac7b2d07bd6042afe0e8618ca8682d7663f4be8
Author: George Rimar <[e-mail]>
Date:   Wed May 29 10:31:46 2019 +0000
    [llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.
    Differential revision: https://reviews.llvm.org/D62179
    llvm-svn: 361943

llvm-readobj cannot print .dynsym of executable without Program Headers. GNU readelf can print it fine. We have found it with @kwk because of some unrelated bug in obj2yaml/`yaml2obj' dropping Program Headers.
On 64-bit Linux one can create a test binary with:

cp -p /bin/bash /tmp/bashtest;dd if=/dev/zero of=/tmp/bashtest bs=1 count=2 seek=$[0x38] conv=notrunc

llvm-readobj -dyn-symbols formerly printed for example:

Symbol {
  Name: tilde_expand_word (11424)
  Value: 0xDBE60

while now it prints only:

warning: Unable to parse DT_GNU_HASH: Virtual address is not in any segment
warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
warning: Unable to parse DT_SYMTAB: Virtual address is not in any segment
warning: Unable to parse DT_JMPREL: Virtual address is not in any segment
warning: Unable to parse DT_RELA: Virtual address is not in any segment
File: /tmp/bashtest
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit 
LoadName:
DynamicSymbols [
]

[grimar]

In D62179#1652778, @jankratochvil wrote:

There is a regression - after this patch - at least after its later commit

I'll investigate.

[grimar]

In D62179#1652778, @jankratochvil wrote:

There is a regression - after this patch - at least after its later commit:

commit 8ac7b2d07bd6042afe0e8618ca8682d7663f4be8
Author: George Rimar <[e-mail]>
Date:   Wed May 29 10:31:46 2019 +0000
    [llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.
    Differential revision: https://reviews.llvm.org/D62179
    llvm-svn: 361943

llvm-readobj cannot print .dynsym of executable without Program Headers. GNU readelf can print it fine. We have found it with @kwk because of some unrelated bug in obj2yaml/`yaml2obj' dropping Program Headers.
On 64-bit Linux one can create a test binary with:

cp -p /bin/bash /tmp/bashtest;dd if=/dev/zero of=/tmp/bashtest bs=1 count=2 seek=$[0x38] conv=notrunc

llvm-readobj -dyn-symbols formerly printed for example:

Symbol {
  Name: tilde_expand_word (11424)
  Value: 0xDBE60

while now it prints only:

warning: Unable to parse DT_GNU_HASH: Virtual address is not in any segment
warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
warning: Unable to parse DT_SYMTAB: Virtual address is not in any segment
warning: Unable to parse DT_JMPREL: Virtual address is not in any segment
warning: Unable to parse DT_RELA: Virtual address is not in any segment
File: /tmp/bashtest
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit 
LoadName:
DynamicSymbols [
]

I posted a patch to fix this: https://reviews.llvm.org/D67078. Thanks for reporting!

test/tools/llvm-readobj/elf-dynamic-no-pt-dynamic.test

-# Show that no dumping occurs if there is no PT_DYNAMIC header.
+## Show that dumping occurs even if there is no PT_DYNAMIC header.
+## This is inconsistent with the GNU behavior, but seems to be more reasonable.
 # RUN: yaml2obj %s -o %t.no-phdr
 # RUN: llvm-readobj --dynamic-table %t.no-phdr | FileCheck %s --check-prefix=LLVM
-# RUN: llvm-readelf --dynamic-table %t.no-phdr | FileCheck %s --check-prefix=GNU --allow-empty
+# RUN: llvm-readelf --dynamic-table %t.no-phdr | FileCheck %s --check-prefix=GNU
 
 # LLVM:      File: {{.*}}.no-phdr
 # LLVM-NEXT: Format: ELF64-x86-64
 # LLVM-NEXT: Arch: x86_64
 # LLVM-NEXT: AddressSize: 64bit
 # LLVM-NEXT: LoadName:{{ *}}
-# LLVM-NOT:  {{.}}
+# LLVM-NEXT: DynamicSection [ (1 entries)
+# LLVM-NEXT:   Tag                Type Name/Value
+# LLVM-NEXT:   0x0000000000000000 NULL 0x0
+# LLVM-NEXT: ]
 
-# GNU-NOT: {{.}}
+# GNU:      DynamicSection [ (1 entries)
+# GNU-NEXT:   Tag                Type Name/Value
+# GNU-NEXT:   0x0000000000000000 NULL 0x0
+# GNU-NEXT: ]
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

tools/llvm-readobj/ELFDumper.cpp

static const char *getElfMipsOptionsOdkType(unsigned Odk) {
   LLVM_READOBJ_ENUM_CASE(ELF, ODK_PAGESIZE);
   default:
     return "Unknown";
   }
 }
 
 template <typename ELFT>
 void ELFDumper<ELFT>::loadDynamicTable(const ELFFile<ELFT> *Obj) {
+  // Try to locate the PT_DYNAMIC header.
   const Elf_Phdr *DynamicPhdr = nullptr;
   for (const Elf_Phdr &Phdr : unwrapOrError(Obj->program_headers())) {
     if (Phdr.p_type != ELF::PT_DYNAMIC)
       continue;
     DynamicPhdr = &Phdr;
     break;
   }
 
-  // We do not want to dump dynamic section if we have no PT_DYNAMIC header.
-  // This matches GNU's behavior.
-  if (!DynamicPhdr)
-    return;
-
   // Try to locate the .dynamic section in the sections header table.
   const Elf_Shdr *DynamicSec = nullptr;
   for (const Elf_Shdr &Sec : unwrapOrError(Obj->sections())) {
     if (Sec.sh_type != ELF::SHT_DYNAMIC)
       continue;
     DynamicSec = &Sec;
     break;
   }
 
   // Information in the section header has priority over the information
   // in a PT_DYNAMIC header.
   // Ignore sh_entsize and use the expected value for entry size explicitly.
   // This allows us to dump the dynamic sections with a broken sh_entsize
   // field.
-  if (DynamicSec)
+  if (DynamicSec) {
     DynamicTable = checkDRI({ObjF->getELFFile()->base() + DynamicSec->sh_offset,
                              DynamicSec->sh_size, sizeof(Elf_Dyn)});
+    parseDynamicTable();
+  }
+
+  // If we have a PT_DYNAMIC header, we will either check the found dynamic
+  // section or take the dynamic table data directly from the header.
+  if (!DynamicPhdr)
+    return;
 
   if (DynamicPhdr->p_offset + DynamicPhdr->p_filesz >
       ObjF->getMemoryBufferRef().getBufferSize())
     reportError(
         "PT_DYNAMIC segment offset + size exceeds the size of the file");
 
   if (!DynamicSec) {
     DynamicTable = createDRIFrom(DynamicPhdr, sizeof(Elf_Dyn));
     parseDynamicTable();
     return;
   }
 
   StringRef Name = unwrapOrError(Obj->getSectionName(DynamicSec));
-
   if (DynamicSec->sh_addr + DynamicSec->sh_size >
           DynamicPhdr->p_vaddr + DynamicPhdr->p_memsz ||
       DynamicSec->sh_addr < DynamicPhdr->p_vaddr)
     reportWarning("The SHT_DYNAMIC section '" + Name +
                   "' is not contained within the "
                   "PT_DYNAMIC segment");
 
   if (DynamicSec->sh_addr != DynamicPhdr->p_vaddr)
     reportWarning("The SHT_DYNAMIC section '" + Name +
                   "' is not at the start of "
                   "PT_DYNAMIC segment");
-
-  parseDynamicTable();
 }
 
 template <typename ELFT>
 ELFDumper<ELFT>::ELFDumper(const object::ELFObjectFile<ELFT> *ObjF,
     ScopedPrinter &Writer)
     : ObjDumper(Writer), ObjF(ObjF) {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
 
template <typename ELFT> void ELFDumper<ELFT>::parseDynamicTable() {
 
   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:
       HashTable =
           reinterpret_cast<const Elf_Hash *>(toMappedAddr(Dyn.getPtr()));
       break;
     case ELF::DT_GNU_HASH:
       GnuHashTable =
           reinterpret_cast<const Elf_GnuHash *>(toMappedAddr(Dyn.getPtr()));
       break;
     case ELF::DT_STRTAB:
       StringTableBegin =
           reinterpret_cast<const char *>(toMappedAddr(Dyn.getPtr()));