[PATCH] D69399: [yaml2obj/obj2yaml] - Add support for SHT_GNU_HASH section.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1087
+  // We've finished writing the header. Here we write the bloom filter.
+  for (llvm::yaml::Hex64 Val : *Section.BloomFilter)
+    support::endian::write<typename ELFT::uint>(OS, Val,
----------------
jhenderson wrote:
> What if `BloomFilter` is `None`?
It can't be `None` here.
See the code in `validate()` method below: `BloomFilter`, `HashBuckets`, `HashValues` and `Header` must be provided when there is no `Content` and must be used together.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1092
+  // Write an array of hash buckets.
+  for (llvm::yaml::Hex32 Val : *Section.HashBuckets)
+    support::endian::write<uint32_t>(OS, Val, ELFT::TargetEndianness);
----------------
jhenderson wrote:
> Ditto.
Ditto.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1096
+  // Write an array of hash values.
+  for (llvm::yaml::Hex32 Val : *Section.HashValues)
+    support::endian::write<uint32_t>(OS, Val, ELFT::TargetEndianness);
----------------
jhenderson wrote:
> Ditto.
Ditto.


================
Comment at: llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml:124
+    HashValues:  []
+## Case 5: Check that when hash values data size contains a set of 4 byte words,
+## we use the "HashValues" property to dump it, but fallback to dumping the whole
----------------
jhenderson wrote:
> The examples to me don't specifically seem to be about the HashValues field? Surely this is just testing that the Content size is a multiple of 4?
The code in `dumpGnuHashSection` never checks that
the whole section data size is or isn't a multiple of 4 atm.

After reading the header, Bloom filter and buckets we have a piece of data left in the section.
(I called it "hash values data" in this comment)
It is assumed to contain hash values and hence it's size should be multiple of 4.
Only then I read it as a set of hash values:

```
ELFDumper<ELFT>::dumpGnuHashSection(const Elf_Shdr *Shdr) {
.....
  S->HashBuckets.emplace(NBuckets);
  for (llvm::yaml::Hex32 &Val : *S->HashBuckets)
    Val = Data.getU32(Cur);

  S->HashValues.emplace((Data.getData().size() - Cur.tell()) / 4);
  for (llvm::yaml::Hex32 &Val : *S->HashValues)
    Val = Data.getU32(Cur);
```

In theory, on this step if hash values data size is not a multiple of 4, we can read and dump it as a raw data,
but still emit the header, filter and buckets fields. I do not think it worth to do, but this is why I think
this is related to the "HashValues" field.



================
Comment at: llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml:211-213
+# RUN: not yaml2obj --docnum=10 %s -o %t10 2>&1 | FileCheck %s --check-prefix=NOKEYS
+
+# NOKEYS: error: either "Content" or "Header", "BloomFilter", "HashBuckets" and "HashBuckets" must be specified
----------------
jhenderson wrote:
> This case should probably be broken into four (or more) separate cases: 1 tag missing for each of the four different tags in the group. As things stand, you don't show that BloomFilter is required if Header is specified, for example.
Ok. This test actually is a bit different, it's intention is to check that either "Content" or "Header", "BloomFilter", "HashBuckets" and "HashBuckets" must be specified. It must have no fields in the section description.

What you describe was supposed to be tested above (--docnums 4 to 7). Though YAMLs there had only a single tag, what probably did not test
the idea you describe properly. I've changed them to have 3 of 4 fields in each YAML. (And this helped to fix a mistype bug :), thanks!)



================
Comment at: llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml:227-229
+# RUN: not yaml2obj --docnum=11 %s -o %t11 2>&1 | FileCheck %s --check-prefix=TOGETHER
+
+# TOGETHER: error: "Header", "BloomFilter", "HashBuckets" and "HashValues" can't be used together with "Content"
----------------
jhenderson wrote:
> Similar to the above, this case should probably be broken up to show that each of the tags individually can't be used with Content.
Probably no, because here we check the following error message:
'error: "Header", "BloomFilter", "HashBuckets" and "HashValues" can't be used together with "Content"'

If I splitted this case it would be impossible to trigger it, because another message would be shown:
'"Header", "BloomFilter", "HashBuckets" and "HashValues" must be used together'

And for the latter message I am already testing each field separatelly above.




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

https://reviews.llvm.org/D69399