[PATCH] D69399: [yaml2obj/obj2yaml] - Add support for SHT_GNU_HASH section.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list