[PATCH] D64750: [llvm-readelf] - Remove the precompiled binary from gnu-hash-symbols.test.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 18 01:51:25 PDT 2019


grimar planned changes to this revision.
grimar marked an inline comment as done.
grimar added a comment.

Plan a minor change.



================
Comment at: test/tools/llvm-readobj/elf-hash-symbols.test:48
+      - Tag:   DT_GNU_HASH
+## 0x2C + PT_LOAD's p_offset == offset of .gnu.hash
+        Value: 0x000000000000002C
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > grimar wrote:
> > > > grimar wrote:
> > > > > MaskRay wrote:
> > > > > > jhenderson wrote:
> > > > > > > Perhaps you should explain what 0x2C is (I think it's the size of .hash, right?)
> > > > > > > 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`.
> > > > > > 
> > > > > 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".
> > > > >> 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.
> > > 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.
> > 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.
> > 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?
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64750/new/

https://reviews.llvm.org/D64750





More information about the llvm-commits mailing list