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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-hash-symbols.test:4
+# RUN: yaml2obj --docnum=1 %s -o %t1-386.o
+# RUN: llvm-readelf --hash-symbols %t1-386.o | FileCheck %s --check-prefix HASH-I386
+
----------------
jhenderson wrote:
> I wonder if we should add --strict-whitespace/--match-full-lines to this. That will ensure spacing is correct. What do you think?
Yep, makes sense, done.


================
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
----------------
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".


================
Comment at: test/tools/llvm-readobj/elf-hash-symbols.test:78
+
+## Show that if there are no hash sections, we do not print anything.
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
----------------
MaskRay wrote:
> jhenderson wrote:
> > Do you think there's value for testing the case with exactly one of .hash or .gnu.hash?
> I think it is fine not to test that...
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).


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

https://reviews.llvm.org/D64750