This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Jul 16 2019, 2:09 AM.

Diff Detail

Event Timeline

grimar created this revision.Jul 16 2019, 2:09 AM
grimar updated this revision to Diff 210049.Jul 16 2019, 2:11 AM
  • Updated comments.
jhenderson added inline comments.Jul 16 2019, 2:29 AM
test/tools/llvm-readobj/elf-hash-symbols.test
78

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

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

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

grimar marked an inline comment as done.Jul 16 2019, 2:43 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-hash-symbols.test
120

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 added inline comments.Jul 16 2019, 3:04 AM
test/tools/llvm-readobj/elf-hash-symbols.test
120

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 marked an inline comment as done.Jul 16 2019, 3:22 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-hash-symbols.test
120

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 added inline comments.Jul 16 2019, 7:44 AM
test/tools/llvm-readobj/elf-hash-symbols.test
78

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

155

%t3.so

grimar updated this revision to Diff 210534.Jul 18 2019, 5:46 AM
grimar marked 3 inline comments as done and an inline comment as not done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
MaskRay accepted this revision.Jul 18 2019, 5:56 AM
This revision is now accepted and ready to land.Jul 18 2019, 5:56 AM
jhenderson added inline comments.Jul 18 2019, 5:58 AM
test/tools/llvm-readobj/elf-hash-symbols.test
78

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 updated this revision to Diff 210574.Jul 18 2019, 8:05 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-hash-symbols.test
78

Removed.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 3:15 AM