This is an archive of the discontinued LLVM Phabricator instance.

Differential D64750

[llvm-readelf] - Remove the precompiled binary from gnu-hash-symbols.test.
ClosedPublic

Authored by grimar on Jul 15 2019, 9:01 AM.

Details

Reviewers
jhenderson
MaskRay
Commits
rG37d7cb2df0c4: [llvm-readelf] - Remove the precompiled binary from gnu-hash-symbols.test
rL366430: [llvm-readelf] - Remove the precompiled binary from gnu-hash-symbols.test
Summary

I am working on https://bugs.llvm.org/show_bug.cgi?id=42622
and this patch reworks the gnu-hash-symbols.test so that it
will be easier to expand it with x86_64 case.

Diff Detail

Event Timeline

grimar created this revision.Jul 15 2019, 9:01 AM
Herald added a subscriber: rupprecht. · View Herald TranscriptJul 15 2019, 9:01 AM
MaskRay added inline comments.Jul 15 2019, 11:27 PM
test/tools/llvm-readobj/gnu-hash-symbols.test
1

Rename gnu-hash-symbols.test to elf-hash-symbols.test (or other better name)

This files tests both .hash (SysV hash) and .gnu.hash (GNU hash).

32

Here and below it -> Here and below

Or better, just explicitly mention that the contents of .hash and .gnu.hash were generated by a linker.

grimar updated this revision to Diff 210041.Jul 16 2019, 1:17 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
grimar added a child revision: D64788: [llvm-readelf] - A fix for: "--hash-symbols asserts for 64-bit ELFs".Jul 16 2019, 2:09 AM
jhenderson added inline comments.Jul 16 2019, 2:25 AM
test/tools/llvm-readobj/elf-hash-symbols.test
4 ↗(On Diff #210041)

I wonder if we should add --strict-whitespace/--match-full-lines to this. That will ensure spacing is correct. What do you think?

20 ↗(On Diff #210041)

I think you need to test that there's nothing after the last line, to show that there are no more entries in .gnu.hash.

48 ↗(On Diff #210041)

Perhaps you should explain what 0x2C is (I think it's the size of .hash, right?)

78 ↗(On Diff #210041)

Do you think there's value for testing the case with exactly one of .hash or .gnu.hash?

MaskRay added inline comments.Jul 16 2019, 3:03 AM
test/tools/llvm-readobj/elf-hash-symbols.test
3 ↗(On Diff #210041)

%t1-386.o -> %t1-386.so

It is weird to name an ET_DYN *.o

48 ↗(On Diff #210041)

PT_LOAD's p_offset

Nit: I think the usage here is a bit unusual. Normally a PT_DYNAMIC is created to contain .dynamic and at runtime the rtld decodes .dynamic entries by finding the PT_DYNAMIC address.

Maybe you can create a PT_DYNAMIC to make the test look more reasonable?

If you decide to do that, delete - Section: .dynamic from the PT_LOAD.

78 ↗(On Diff #210041)

I think it is fine not to test that...

grimar updated this revision to Diff 210067.Jul 16 2019, 5:07 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-hash-symbols.test
4 ↗(On Diff #210041)

Yep, makes sense, done.

48 ↗(On Diff #210041)

I expanded the comment to reveal the full picture of how the computation happen.

Note that in this test case both .hash and .gnu.hash have section VAs = 0
(yaml2obj doesn't set it). So I tried to explain the underhood without using
a wordings like "address of hash section".

78 ↗(On Diff #210041)

I do not mind. Done.

Side question: if we have a file with dynamic table that has DT_HASH and DT_GNU_HASH tags
and we have .hash and .gnu.hash sections. Do you think that llvm-objcopy should remove DT_HASH if we remove a .hash section?
(Does not seem GNU objcopy do that, I only did a quick test though, doesn't seem it is correct to keep it).

grimar planned changes to this revision.Jul 16 2019, 5:08 AM

I did not see MaskRay`s comments. Going to address them.

grimar updated this revision to Diff 210069.Jul 16 2019, 5:13 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-hash-symbols.test
3 ↗(On Diff #210041)

Done.

48 ↗(On Diff #210041)

Nit: I think the usage here is a bit unusual. Normally a PT_DYNAMIC is created to contain .dynamic and at runtime the rtld decodes .dynamic entries by finding the PT_DYNAMIC address.

But we are testing here the logic of llvm-readelf. It does not need PT_DYNAMIC to dump the hash tables.

grimar marked an inline comment as done.Jul 16 2019, 5:27 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-hash-symbols.test
48 ↗(On Diff #210041)

Moreover, I think we could improve dumper to be able to dump these section even without any .dynamic section at all.
They have the unique types: SHT_HASH and SHT_GNU_HASH. It should be possible do do that.

MaskRay added inline comments.Jul 16 2019, 7:34 AM
test/tools/llvm-readobj/elf-hash-symbols.test
218 ↗(On Diff #210069)

%t4.so

48 ↗(On Diff #210041)

Not having PT_DYNAMIC should be fine.

One more nit: it made me puzzled because .dynamic is writable and contained in the PT_LOAD but the PT_LOAD is not writable..

You can delete the SHF_WRITE flag from .dynamic (it doesn't include DT_DEBUG so it is not necessarily writable).

Moreover, I think we could improve dumper to be able to dump these section even without any .dynamic section at all.
They have the unique types: SHT_HASH and SHT_GNU_HASH. It should be possible do do that.

GNU objcopy just locates the dynamic section with either PT_DYNAMIC or .dynamic, then decodes DT_HASH and/or DT_GNU_HASH. It doesn't check SHT_HASH or SHT_GNU_HASH probably because the section header is optional in ET_DYN/ET_EXEC. However, PT_DYNAMIC must be available for the hash tables to be recognized by ld.so, so objcopy doesn't bother implementing the logic.

jhenderson added inline comments.Jul 16 2019, 7:53 AM
test/tools/llvm-readobj/elf-hash-symbols.test
7 ↗(On Diff #210069)

HASH-I386 -> HASH-32

78 ↗(On Diff #210041)

I don't think llvm-objcopy should be modifying the contents of the .dynamic section or any other loadable section, with the exception of removing the contents of sections where we explicitly request to remove them. Doing so can mess up addresses and so on.

grimar updated this revision to Diff 210380.Jul 17 2019, 11:31 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-hash-symbols.test
218 ↗(On Diff #210069)

Oh, thanks.

48 ↗(On Diff #210041)

GNU objcopy just locates the dynamic section with either PT_DYNAMIC or .dynamic, then decodes DT_HASH and/or DT_GNU_HASH. It doesn't check SHT_HASH or SHT_GNU_HASH probably because the section header is optional in ET_DYN/ET_EXEC. However, PT_DYNAMIC must be available for the hash tables to be recognized by ld.so, so objcopy doesn't bother implementing the logic.

Sorry if I am missing something, but this patch is for llvm-readelf. Did you mean GNU readelf?
(probably not, because as far I remember GNU readelf does not find .dynamic without PT_DYNAMIC though my memories are not strong right now about the difference we have atm).
But anyways what I was thinking of is the following logic: in llvm-readelf for dumping objects we can:

  1. Try to find .dynamic and DT_HASH/DT_GNU_HASH, read the hash tables on success. (that is what we already do)
  2. If (1) fails, then try to use the SHT_HASH/SHT_GNU_HASH we found (probably) earlier during the sections scan. I.e. read the content directly using sections headers info if available. (new)

That would allow us to dump more information in a broken objects. At least it seems would be useful for the test cases like that.
Does it make sense?

jhenderson accepted this revision.Jul 18 2019, 1:46 AM

LGTM, but obviously wait for @MaskRay's comments.

This revision is now accepted and ready to land.Jul 18 2019, 1:46 AM
grimar planned changes to this revision.Jul 18 2019, 1:51 AM
grimar marked an inline comment as done.

Plan a minor change.

test/tools/llvm-readobj/elf-hash-symbols.test
48 ↗(On Diff #210041)

You can delete the SHF_WRITE flag from .dynamic (it doesn't include DT_DEBUG so it is not necessarily writable).

I think I missed this one. Going to remove it.

MaskRay accepted this revision.Jul 18 2019, 1:57 AM
MaskRay added inline comments.
test/tools/llvm-readobj/elf-hash-symbols.test
48 ↗(On Diff #210041)

If (1) fails, then try to use the SHT_HASH/SHT_GNU_HASH we found (probably) earlier during the sections scan. I.e. read the content directly using sections headers info if available. (new)

My feeling is that just locating DT_HASH and DT_GNU_HASH should be good enough. There may be cases that both 1) PT_DYNAMIC is broken and 2) .dynamic is missing. In these cases, locating SHT_HASH/SHT_GNU_HASH as a fallback will still allow us to dump hash tables (your proposal). My feeling is that these cases may be rare enough that we don't need to do so. (I wanted to save you the work but I will not be opposed to the idea if you insist on implementing it...)

This revision was not accepted when it landed; it landed in state Changes Planned.Jul 18 2019, 5:14 AM
Closed by commit rL366430: [llvm-readelf] - Remove the precompiled binary from gnu-hash-symbols.test (authored by grimar). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 5:14 AM

Revision Contents

PathSize
test/
tools/
llvm-readobj/
Inputs/
110 lines

Diff 209877

test/tools/llvm-readobj/Inputs/dynamic-table-exe.x86

  • This binary file was deleted.
PropertyOld ValueNew Value
File Mode100755null

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

# RUN: llvm-readelf --hash-symbols %p/Inputs/dynamic-table-exe.x86 \ ## Check llvm-readelf is able to dump the content of hash sections correctly.
MaskRayUnsubmitted
Done
ReplyInline Actions

Rename gnu-hash-symbols.test to elf-hash-symbols.test (or other better name)

This files tests both .hash (SysV hash) and .gnu.hash (GNU hash).

MaskRay: Rename `gnu-hash-symbols.test` to `elf-hash-symbols.test` (or other better name) This files…
# RUN: | FileCheck %s --check-prefix HASH
# HASH: Symbol table of .hash for image: # RUN: yaml2obj --docnum=1 %s -o %t1-386.o
# HASH-NEXT: Num Buc: Value Size Type Bind Vis Ndx Name # RUN: llvm-readelf --hash-symbols %t1-386.o | FileCheck %s --check-prefix HASH-I386
# HASH-NEXT: 9 0: 00000000 0 FUNC GLOBAL DEFAULT UND __gxx_personality_v0@CXXABI_1.3
# HASH-NEXT: 13 0: 00001b64 0 NOTYPE GLOBAL DEFAULT ABS _edata{{$}} # HASH-I386: Symbol table of .hash for image:
# HASH-NEXT: 7 0: 00000000 0 FUNC GLOBAL DEFAULT UND _ZNSt14basic_ifstreamIcSt11char_traitsIcEEC1EPKcSt13_Ios_Openmode@GLIBCXX_3.4 # HASH-I386-NEXT: Num Buc: Value Size Type Bind Vis Ndx Name
# HASH-NEXT: 2 0: 00000000 0 NOTYPE WEAK DEFAULT UND _Jv_RegisterClasses{{$}} # HASH-I386-NEXT: 1 0: 00000000 0 NOTYPE GLOBAL DEFAULT UND ccc
# HASH-NEXT: 1 0: 00000000 0 NOTYPE WEAK DEFAULT UND __gmon_start__{{$}} # HASH-I386-NEXT: 5 0: 00001001 0 NOTYPE WEAK DEFAULT 1 bbb
# HASH-NEXT: 16 1: 00000850 81 FUNC GLOBAL DEFAULT 14 main{{$}} # HASH-I386-NEXT: 3 0: 00000001 0 NOTYPE GLOBAL DEFAULT ABS ddd
# HASH-NEXT: 10 1: 00000000 0 FUNC GLOBAL DEFAULT UND _Unwind_Resume@GCC_3.0 # HASH-I386-NEXT: 2 0: 00001000 0 NOTYPE GLOBAL DEFAULT 1 aaa
# HASH-NEXT: 8 1: 00000000 0 FUNC GLOBAL DEFAULT UND puts@GLIBC_2.0 # HASH-I386-NEXT: 4 0: 00000000 0 NOTYPE GLOBAL DEFAULT 2 eee
# HASH-NEXT: 12 1: 00001b68 0 NOTYPE GLOBAL DEFAULT ABS _end{{$}} # HASH-I386-EMPTY:
# HASH-NEXT: 6 1: 00000000 0 FUNC GLOBAL DEFAULT UND _ZNSt14basic_ifstreamIcSt11char_traitsIcEED1Ev@GLIBCXX_3.4 # HASH-I386: Symbol table of .gnu.hash for image:
# HASH-NEXT: 5 1: 00000000 0 NOTYPE WEAK DEFAULT UND _ITM_registerTMCloneTable{{$}} # HASH-I386-NEXT: Num Buc: Value Size Type Bind Vis Ndx Name
# HASH-NEXT: 4 1: 00000000 0 NOTYPE WEAK DEFAULT UND _ITM_deregisterTMCloneTable{{$}} # HASH-I386-NEXT: 2 1: 00001000 0 NOTYPE GLOBAL DEFAULT 1 aaa
# HASH-NEXT: 3 1: 00000000 0 FUNC GLOBAL DEFAULT UND __libc_start_main@GLIBC_2.0 # HASH-I386-NEXT: 3 1: 00000001 0 NOTYPE GLOBAL DEFAULT ABS ddd
# HASH-NEXT: 11 2: 00000000 0 FUNC WEAK DEFAULT UND __cxa_finalize@GLIBC_2.1.3 # HASH-I386-NEXT: 4 2: 00000000 0 NOTYPE GLOBAL DEFAULT 2 eee
# HASH-NEXT: 15 2: 00001b64 0 NOTYPE GLOBAL DEFAULT ABS __bss_start{{$}} # HASH-I386-NEXT: 5 2: 00001001 0 NOTYPE WEAK DEFAULT 1 bbb
# HASH-NEXT: 14 2: 0000093c 4 OBJECT GLOBAL DEFAULT 16 _IO_stdin_used{{$}}
# HASH: Symbol table of .gnu.hash for image: --- !ELF
# HASH-NEXT: Num Buc: Value Size Type Bind Vis Ndx Name FileHeader:
# HASH-NEXT: 12 0: 00001b68 0 NOTYPE GLOBAL DEFAULT ABS _end{{$}} Class: ELFCLASS32
# HASH-NEXT: 13 0: 00001b64 0 NOTYPE GLOBAL DEFAULT ABS _edata{{$}} Data: ELFDATA2LSB
# HASH-NEXT: 14 1: 0000093c 4 OBJECT GLOBAL DEFAULT 16 _IO_stdin_used{{$}} Type: ET_DYN
# HASH-NEXT: 15 1: 00001b64 0 NOTYPE GLOBAL DEFAULT ABS __bss_start{{$}} Machine: EM_386
# HASH-NEXT: 16 1: 00000850 81 FUNC GLOBAL DEFAULT 14 main{{$}} Sections:
- Name: .hash
Type: SHT_HASH
Flags: [ SHF_ALLOC ]
Link: .dynsym
## Here and below it is a linker produced binary content for the symbols under the `DynamicSymbols` tag.
MaskRayUnsubmitted
Done
ReplyInline Actions

Here and below it -> Here and below

Or better, just explicitly mention that the contents of .hash and .gnu.hash were generated by a linker.

MaskRay: `Here and below it` -> `Here and below` Or better, just explicitly mention that the contents…
Content: '0300000006000000010000000000000000000000000000000500000004000000020000000000000003000000'
- Name: .gnu.hash
Type: SHT_GNU_HASH
Flags: [ SHF_ALLOC ]
Link: .dynsym
Content: 0300000002000000010000000500000048991200000000000200000004000000685C880B9169880BF46D880BCB60880B
- Name: .dynamic
Type: SHT_DYNAMIC
Flags: [ SHF_WRITE, SHF_ALLOC ]
Link: .dynstr
Entries:
## 0x0 + PT_LOAD's p_offset == offset of .hash
- Tag: DT_HASH
Value: 0x0000000000000000
- Tag: DT_GNU_HASH
## 0x2C + PT_LOAD's p_offset == offset of .gnu.hash
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
## Show that if there are no hash sections, we do not print anything. ## Show that if there are no hash sections, we do not print anything.
# RUN: yaml2obj %s -o %t.o # RUN: yaml2obj --docnum=2 %s -o %t2.o
# RUN: llvm-readelf --hash-symbols %t.o \ # RUN: llvm-readelf --hash-symbols %t2.o \
# RUN: | FileCheck %s --check-prefix NO-HASH --allow-empty # RUN: | FileCheck %s --check-prefix NO-HASH --allow-empty
# NO-HASH-NOT: {{.}} # NO-HASH-NOT: {{.}}
## Sanity check that we can still find the dynamic symbols (i.e. the above test ## 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). ## doesn't pass due to a mistake in the dynamic section).
# RUN: llvm-readelf --dyn-symbols %t.o | FileCheck %s --check-prefix DYNSYMS # RUN: llvm-readelf --dyn-symbols %t2.o | FileCheck %s --check-prefix DYNSYMS
# DYNSYMS: Symbol table '.dynsym' contains 2 entries: # DYNSYMS: Symbol table '.dynsym' contains 2 entries:
!ELF --- !ELF
FileHeader: FileHeader:
Class: ELFCLASS64 Class: ELFCLASS64
Data: ELFDATA2LSB Data: ELFDATA2LSB
Type: ET_DYN Type: ET_DYN
Machine: EM_X86_64 Machine: EM_X86_64
Sections: Sections:
- Name: .dynstr - Name: .dynstr
Type: SHT_STRTAB Type: SHT_STRTAB
▲ Show 20 Lines • Show All 53 Lines • Show Last 20 Lines