[PATCH] D68085: [yaml2obj/obj2yaml] - Add support for SHT_HASH sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:824-825
+  unsigned Link = 0;
+  if (SN2I.lookup(".dynsym", Link))
+    SHeader.sh_link = Link;
+
----------------
Could it be possible to specify a custom sh_link value?


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:827
+
+  if (Section.Content) {
+    SHeader.sh_size = writeContent(OS, Section.Content, None);
----------------
As with the stack sizes sections, it would be good if a user can specify the size explicitly.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:841
+
+  SHeader.sh_size = (2 + Section.Bucket->size() + Section.Chain->size()) * 4;
+}
----------------
Maybe rather than "4" here, this should be something like `sizeof(Elf32_Word)`?


================
Comment at: test/tools/obj2yaml/elf-hash-section.yaml:26
+Sections:
+## Case 1: A regular case sample: nbucket == 1, nchain == 2. 
+  - Name:    .hash1
----------------
"A non-empty hash table:"


================
Comment at: test/tools/obj2yaml/elf-hash-section.yaml:35
+
+## Check that obj2yaml fallbacks to using "Content" tag when
+## hash sections are broken.
----------------
fallbacks -> falls back
using "Content" -> using the "Content"


================
Comment at: test/tools/obj2yaml/elf-hash-section.yaml:75
+## Case 4: nbucket == 1, nchain == 2.
+##         Section size is grater than (2 * nbucket + nchain) * 4.
+  - Name:    .oversized
----------------
grater -> greater


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:3
+
+## Check we can desctibe SHT_HASH using "Content" tag.
+
----------------
desctibe -> describe
"a SHT_HASH section using the "Content" tag"


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:38
+
+## Check we can desctibe SHT_HASH using "Bucket" and "Chain" tags.
+
----------------
desctibe SHT_HASH -> describe a SHT_HASH section


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:65
+
+## Check we can't use "Content" and "Bucket" tags together.
+
----------------
What about "Content" and "Chain"?


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:69
+
+# CONTENT-BUCKET: error: Content and Bucket cannot be used together
+
----------------
I'd quote the names of the tags in the error messages (like you do in the comment above).


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:115
+
+## Check we report an error if neither "Bucket", "Chain" or "Content" were set.
+
----------------
or -> nor

(use "or" with "either" and "nor" with "neither" to be strictly correct)


================
Comment at: tools/obj2yaml/elf2yaml.cpp:648
+  uint32_t NChain = Data.getUnsigned(Cur, 4);
+  if (Content.size() != (2 + NBucket + NChain) * 4) {
+    S->Content = yaml::BinaryRef(Content);
----------------
`4` -> `sizeof (Elf32_Word)`?


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

https://reviews.llvm.org/D68085