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

Thu Jul 18 01:57:52 PDT 2019

MaskRay accepted this revision.
MaskRay added inline comments.

================
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:
> 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.
> 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...)

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

https://reviews.llvm.org/D64750