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

Mon Oct 28 09:42:09 PDT 2019

jhenderson added a comment.

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

================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:240
+  // Not used when dumping the object, but can be used to override
+  // the real number of buckets when emiting an object from the YAML document.
+  Optional<llvm::yaml::Hex32> NBuckets;
----------------
from the -> from a

================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:244
+  // Index of the first symbol in the dynamic symbol table
+  // included into the hash table.
+  llvm::yaml::Hex32 SymNdx;
----------------
into -> in

================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:249-250
+  // Not used when dumping the object, but can be used to override
+  // the real number of words in the Bloom filter when emiting an object from
+  // the YAML document.
+  Optional<llvm::yaml::Hex32> MaskWords;
----------------
from the -> from a

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1059
+
+  // We write the header first. Here we write hash buckets count. Normally it is
+  // the number of entries in HashBuckets, but "NBuckets" property can be
----------------
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."?

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1060
+  // We write the header first. Here we write hash buckets count. Normally it is
+  // the number of entries in HashBuckets, but "NBuckets" property can be
+  // used to override this field, what is useful for producing broken objects.
----------------
but the "NBuckets"

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1061
+  // the number of entries in HashBuckets, but "NBuckets" property can be
+  // used to override this field, what is useful for producing broken objects.
+  if (Section.Header->NBuckets)
----------------
what -> which

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1069
+
+  // Write index of the first symbol in the dynamic symbol table.
+  support::endian::write<uint32_t>(OS, Section.Header->SymNdx,
----------------
I'd expand this comment a bit:

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

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1073
+
+  // Write the number of words in the Bloom filter. Just like above, the
+  // "MaskWords" property can be used to put any value to this field.
----------------
Just like above -> As above

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1074
+  // Write the number of words in the Bloom filter. Just like above, the
+  // "MaskWords" property can be used to put any value to this field.
+  if (Section.Header->MaskWords)
----------------
to put any value to this field -> to set this field to any value

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1086
+
+  // We've finished writing the header. Here we write the bloom filter.
+  for (llvm::yaml::Hex64 Val : *Section.BloomFilter)
----------------
bloom -> Bloom

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

================
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,
----------------
What if `BloomFilter` is `None`?

================
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);
----------------
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);
----------------
Ditto.

================
Comment at: llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml:86-87
+## It is almost technically valid, but uncommon. Modern linkers
+## create at least one entry in bloom filter if they want to disable it,
+## also the dynamic symbol table has a null entry and having SymNdx = 0
+## here is at least strange.
----------------
it, also -> it. Also,

================
Comment at: llvm/test/tools/obj2yaml/elf-gnu-hash-section.yaml:100
+    HashValues:  []
+## Case 3: MaskWords field is broken, it says that the number of entries
+## in the bloom filter is 1, but the bloom filter is empty.
----------------
broken, it -> broken: it

================
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
----------------
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?

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

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

================
Comment at: llvm/test/tools/yaml2obj/elf-gnu-hash-section.yaml:248
+
+## Test we can override the number of buckets and the number of words in the bloom filter
+## using the "NBuckets" and "Shift2" keys.
----------------
bloom -> Bloom

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

https://reviews.llvm.org/D69399