This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj/obj2yaml] - Add support for SHT_GNU_HASH section.
ClosedPublic

Authored by grimar on Oct 24 2019, 10:49 AM.

Details

Summary

This adds parsing and dumping support for GNU hash sections.
They are described nicely here: https://blogs.oracle.com/solaris/gnu-hash-elf-sections-v2

Depends on https://reviews.llvm.org/D69558

Diff Detail

Event Timeline

grimar created this revision.Oct 24 2019, 10:49 AM
jhenderson added a comment.EditedOct 28 2019, 9:42 AM

The changes related to the LLVM_YAML_IS_SEQUENCE_VECTOR seems unrelated to the hash table stuff. Could it be a separate prerequisite change?

llvm/include/llvm/ObjectYAML/ELFYAML.h
256

from the -> from a

260

into -> in

265–266

from the -> from a

llvm/lib/ObjectYAML/ELFEmitter.cpp
1066

This reads a bit clunky to me. How about changing the first two sentences to "We write the header first, starting with the hash buckets count."?

1067

but the "NBuckets"

1068

what -> which

1076

I'd expand this comment a bit:

"Write the index of the first symbol in the dynamic symbol table accessible via the hash table."

1080

Just like above -> As above

1081

to put any value to this field -> to set this field to any value

1093

bloom -> Bloom

Perhaps change "Here we write" to "Now write"

1094

What if BloomFilter is None?

1099

Ditto.

1103

Ditto.

llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml
87–88

it, also -> it. Also,

101

broken, it -> broken: it

125

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?

llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml
212–214

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.

228–230

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.

249

bloom -> Bloom

grimar updated this revision to Diff 226895.Oct 29 2019, 6:58 AM
grimar marked 23 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressd review comments.
llvm/lib/ObjectYAML/ELFEmitter.cpp
1094

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.

1099

Ditto.

1103

Ditto.

llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml
125

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.

llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml
212–214

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!)

228–230

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.

jhenderson added inline comments.Oct 29 2019, 7:59 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
1094

Right, sorry, got my timeline wrong on what methods can get called when.

llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml
88

"Also the" -> "Also, the"

125

Okay, I think this comment needs rephrasing. How about:

"Check that we use the various properties to dump the data when it has a size that is a multiple of 4, but fallback to..."

llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml
138

.gnu.hash.no.header?

151

.gnu.hash.no.bloomfilter?

166

.gnu.hash.no.nobuckets?

181

.gnu.hash.no.novalues?

261

This bloom hasn't been changed to Bloom.

grimar updated this revision to Diff 227065.Oct 30 2019, 5:17 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.
llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml
138

Right, thanks.

261

Done. Sorry.

This revision is now accepted and ready to land.Oct 30 2019, 5:35 AM
This revision was automatically updated to reflect the committed changes.