[llvm-readelf] - A fix for: "--hash-symbols asserts for 64-bit ELFs"

[grimar]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=42622.
(--hash-symbols switch is currently broken for 64-bit ELF files, due to r352630.)

[grimar]

• Updated comments.

[jhenderson]

test/tools/llvm-readobj/elf-hash-symbols.test

78 ↗

(On Diff #210049)

I don't think it's the x86_64 bit that's important, if I'm not mistaken, merely the 64-bit nature of the ELF, so perhaps simplify the above i386 naming to simply "32" and this x86_64 to simply "64".

120 ↗

(On Diff #210049)

Why is this 0x2be, rather than 0x0 like in the 32-bit case?

Worth a comment saying what "0x2b8" (and "0x2e4" below) represent.

[grimar]

test/tools/llvm-readobj/elf-hash-symbols.test

120 ↗

(On Diff #210049)

In this case I set Offset to 0 for PT_LOAD. And so 0x2b8 and 0x2e4 are the offsets of the hash sections:

Section Headers:

[Nr] Name              Type             Address           Offset
     Size              EntSize          Flags  Link  Info  Align

...

[ 1] .hash             HASH             0000000000000000  000002b8
     000000000000002c  0000000000000000   A       7     0     0
[ 2] .gnu.hash         GNU_HASH         0000000000000000  000002e4
     0000000000000034  0000000000000000   A       7     0     0

Program Headers:

Type           Offset             VirtAddr           PhysAddr
               FileSiz            MemSiz              Flags  Align
LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
               0x0000000000000348 0x0000000000000348  R E    0x1

In 32-bit case I did not set Offset, so it was not 0.
I thought my comments saying something like 0x2b8 + PT_LOAD's p_offset == offset of .hash are good enough,
as it does not really important what is 0x2b8, only the result of the formula is important.

I do not mind expanding the comment.
Which approach would you prefer? (used in 32 bit or 64 bit case, i.e. with Offset set to 0 or not).

[jhenderson]

test/tools/llvm-readobj/elf-hash-symbols.test

120 ↗

(On Diff #210049)

I think I'd prefer the Offset to not be explicitly set, i.e. the 32-bit approach.

By the way, the comment is slightly misleading, because the dynamic tag value is the address, not the offset, so really it should be referencing the program header's p_vaddr field.

[grimar]

test/tools/llvm-readobj/elf-hash-symbols.test

120 ↗

(On Diff #210049)

It is a bit more complex actually.

Both are used to read the hash sections data:
https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L581

uint64_t Delta = VAddr - Phdr.p_vaddr;
if (Delta >= Phdr.p_filesz)
  return createError("virtual address is not in any segment: 0x" +
                     Twine::utohexstr(VAddr));
return base() + Phdr.p_offset + Delta;

But program header's p_vaddr is null here, so I mentioned only p_offset field.

[MaskRay]

test/tools/llvm-readobj/elf-hash-symbols.test

78 ↗

(On Diff #210049)

Just a reminder: .so (or just delete the .o suffix)

155 ↗

(On Diff #210049)

%t3.so

[grimar]

• Addressed review comments.

[jhenderson]

test/tools/llvm-readobj/elf-hash-symbols.test

228 ↗

(On Diff #210534)

I don't think you need 64-bit versions of the "ONLY-HASH/ONLY-GNUHASH" cases, as long as there's one for the main case.

[grimar]

• Addressed review comments.

test/tools/llvm-readobj/elf-hash-symbols.test

228 ↗

(On Diff #210534)

Removed.

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-readobj/elf-hash-symbols.test

 ProgramHeaders:
   - Type:  PT_LOAD
     Flags: [ PF_R, PF_X ]
     Sections:
       - Section: .hash
       - Section: .gnu.hash
       - Section: .dynamic
 
+# RUN: yaml2obj --docnum=2 %s -o %t1-64.so
+# RUN: llvm-readelf --hash-symbols %t1-64.so | FileCheck %s --check-prefix HASH-64
+
+# HASH-64:      Symbol table of .hash for image:
+# HASH-64-NEXT:  Num Buc:    Value          Size   Type   Bind Vis      Ndx Name
+# HASH-64-NEXT:    1   0: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT UND ccc
+# HASH-64-NEXT:    5   0: 0000000000001001     0 NOTYPE  WEAK   DEFAULT   1 bbb
+# HASH-64-NEXT:    3   0: 0000000000000001     0 NOTYPE  GLOBAL DEFAULT ABS ddd
+# HASH-64-NEXT:    2   0: 0000000000001000     0 NOTYPE  GLOBAL DEFAULT   1 aaa
+# HASH-64-NEXT:    4   0: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT   2 eee
+# HASH-64-EMPTY:
+# HASH-64-NEXT: Symbol table of .gnu.hash for image:
+# HASH-64-NEXT:  Num Buc:    Value          Size   Type   Bind Vis      Ndx Name
+# HASH-64-NEXT:    2   1: 0000000000001000     0 NOTYPE  GLOBAL DEFAULT   1 aaa
+# HASH-64-NEXT:    3   1: 0000000000000001     0 NOTYPE  GLOBAL DEFAULT ABS ddd
+# HASH-64-NEXT:    4   2: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT   2 eee
+# HASH-64-NEXT:    5   2: 0000000000001001     0 NOTYPE  WEAK   DEFAULT   1 bbb
+# HASH-64-NOT: {{.}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+## The contents of .hash and .gnu.hash were generated by a linker.
+  - Name:    .hash
+    Type:    SHT_HASH
+    Flags:   [ SHF_ALLOC ]
+    Link:    .dynsym
+    Content: '0300000006000000010000000000000000000000000000000500000004000000020000000000000003000000'
+  - Name:    .gnu.hash
+    Type:    SHT_GNU_HASH
+    Flags:   [ SHF_ALLOC ]
+    Link:    .dynsym
+    Content: 030000000200000001000000060000000808020040019200000000000200000004000000685C880B9169880BF46D880BCB60880B
+  - Name:  .dynamic
+    Type:  SHT_DYNAMIC
+    Flags: [ SHF_ALLOC ]
+    Link:  .dynstr
+    Entries:
+      - Tag:   DT_HASH
+## PT_LOAD's p_vaddr is 0x0. PT_LOAD's p_offset = 0x2b8. DT_HASH value is 0x0. 
+## llvm-readelf will read .hash content from p_offset + (p_vaddr - DT_HASH value) = 0x2b8.
+## This matches the file offset of the .hash section. 
+        Value: 0x0000000000000000
+      - Tag:   DT_GNU_HASH
+## PT_LOAD's p_vaddr is 0x0. PT_LOAD's p_offset = 0x2b8. DT_GNU_HASH value is 0x2c (size of .hash = 0x2c).
+## llvm-readelf will read .gnu.hash content from p_offset + (p_vaddr - DT_GNU_HASH value) = 0x2e4.
+## This matches the file offset of the .gnu.hash section.
+        Value: 0x000000000000002c
+      - Tag:   DT_NULL
+        Value: 0x0000000000000000
+DynamicSymbols:
+  - Name:    ccc
+    Binding: STB_GLOBAL
+  - Name:    aaa
+    Section: .hash
+    Binding: STB_GLOBAL
+    Value:   0x0000000000001000
+  - Name:    ddd
+    Index:   SHN_ABS
+    Binding: STB_GLOBAL
+    Value:   0x0000000000000001
+  - Name:    eee
+    Section: .gnu.hash
+    Binding: STB_GLOBAL
+  - Name:    bbb
+    Section: .hash
+    Binding: STB_WEAK
+    Value:   0x0000000000001001
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Flags: [ PF_R, PF_X ]
+    Sections:
+      - Section: .hash
+      - Section: .gnu.hash
+      - Section: .dynamic
+
 ## Check the output when only .hash section is present.
 
-# RUN: yaml2obj --docnum=2 %s -o %t2-32.so
+# RUN: yaml2obj --docnum=3 %s -o %t2-32.so
 # RUN: llvm-readelf --hash-symbols %t2-32.so \
 # RUN:   | FileCheck %s --strict-whitespace --match-full-lines --check-prefix ONLY-HASH-32
 
 # ONLY-HASH-32: Symbol table of .hash for image:
 # ONLY-HASH-32-NEXT:  Num Buc:    Value  Size   Type   Bind Vis      Ndx Name
 # ONLY-HASH-32-NEXT:    1   0: 00000000     0 NOTYPE  GLOBAL DEFAULT UND ccc
 # ONLY-HASH-32-NEXT:    5   0: 00001001     0 NOTYPE  WEAK   DEFAULT   1 bbb
 # ONLY-HASH-32-NEXT:    3   0: 00000001     0 NOTYPE  GLOBAL DEFAULT ABS ddd
ProgramHeaders:
   - Type:  PT_LOAD
     Flags: [ PF_R, PF_X ]
     Sections:
       - Section: .hash
       - Section: .dynamic
 
 ## Check the output when only .gnu.hash section is present.
 
-# RUN: yaml2obj --docnum=3 %s -o %t3-32.so
+# RUN: yaml2obj --docnum=4 %s -o %t3-32.so
 # RUN: llvm-readelf --hash-symbols %t3-32.so \
 # RUN:   | FileCheck %s --strict-whitespace --match-full-lines --check-prefix ONLY-GNUHASH-32
 
 # ONLY-GNUHASH-32: Symbol table of .gnu.hash for image:
 # ONLY-GNUHASH-32-NEXT:  Num Buc:    Value  Size   Type   Bind Vis      Ndx Name
 # ONLY-GNUHASH-32-NEXT:    2   1: 00001000     0 NOTYPE  GLOBAL DEFAULT   2 aaa
 # ONLY-GNUHASH-32-NEXT:    3   1: 00000001     0 NOTYPE  GLOBAL DEFAULT ABS ddd
 # ONLY-GNUHASH-32-NEXT:    4   2: 00000000     0 NOTYPE  GLOBAL DEFAULT   1 eee

 ProgramHeaders:
   - Type:  PT_LOAD
     Flags: [ PF_R, PF_X ]
     Sections:
       - Section: .gnu.hash
       - Section: .dynamic
 
 ## Show that if there are no hash sections, we do not print anything.
-# RUN: yaml2obj --docnum=4 %s -o %t4.so
+# RUN: yaml2obj --docnum=5 %s -o %t4.so
 # RUN: llvm-readelf --hash-symbols %t4.so \
 # RUN:   | FileCheck %s --check-prefix NO-HASH --allow-empty
 
 # NO-HASH-NOT: {{.}}
 
 ## Sanity check that we can still find the dynamic symbols (i.e. the above test
 ## doesn't pass due to a mistake in the dynamic section).
 # RUN: llvm-readelf --dyn-symbols %t4.so | FileCheck %s --check-prefix DYNSYMS

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

void GNUStyle<ELFT>::printHashedSymbol(const ELFO *Obj, const Elf_Sym *FirstSym,
   unsigned Bias = ELFT::Is64Bits ? 8 : 0;
   Field Fields[9] = {0,         6,         11,        20 + Bias, 25 + Bias,
                      34 + Bias, 41 + Bias, 49 + Bias, 53 + Bias};
   Fields[0].Str = to_string(format_decimal(Sym, 5));
   Fields[1].Str = to_string(format_decimal(Bucket, 3)) + ":";
 
   const auto Symbol = FirstSym + Sym;
   Fields[2].Str = to_string(
-      format_hex_no_prefix(Symbol->st_value, ELFT::Is64Bits ? 18 : 8));
+      format_hex_no_prefix(Symbol->st_value, ELFT::Is64Bits ? 16 : 8));
   Fields[3].Str = to_string(format_decimal(Symbol->st_size, 5));
 
   unsigned char SymbolType = Symbol->getType();
   if (Obj->getHeader()->e_machine == ELF::EM_AMDGPU &&
       SymbolType >= ELF::STT_LOOS && SymbolType < ELF::STT_HIOS)
     Fields[4].Str = printEnum(SymbolType, makeArrayRef(AMDGPUSymbolTypes));
   else
     Fields[4].Str = printEnum(SymbolType, makeArrayRef(ElfSymbolTypes));