Page MenuHomePhabricator

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

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

Details

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 15 2019, 9:01 AM
MaskRay added inline comments.Jul 15 2019, 11:27 PM
test/tools/llvm-readobj/gnu-hash-symbols.test
1 ↗(On Diff #209877)

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 ↗(On Diff #209877)

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.
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
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 5:14 AM