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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:824-825
+  unsigned Link = 0;
+  if (SN2I.lookup(".dynsym", Link))
+    SHeader.sh_link = Link;
+
----------------
jhenderson wrote:
> Could it be possible to specify a custom sh_link value?
I prepared a follow-up for this: D68214


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


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:841
+
+  SHeader.sh_size = (2 + Section.Bucket->size() + Section.Chain->size()) * 4;
+}
----------------
jhenderson wrote:
> Maybe rather than "4" here, this should be something like `sizeof(Elf32_Word)`?
I wouldn't do that. Spec says: "A hash table consists of Elf32_Word or Elf64_Word objects that provide for symbol table access."
I.e. we could use `sizeof(typename ELFT::Word)` here, but given that `sizeof(Elf32_Word) == sizeof(Elf64_Word) == 4`, what is the profit?
It does not make the code cleaner or more readable, using `4` instead looks much simpler. 
For the same reason I used `support::endian::write<uint32_t>` instead of possible `support::endian::write<typename ELFT::Word>`

FWIW in LLD we do the same sometimes: https://github.com/llvm-mirror/lld/blob/master/ELF/SyntheticSections.cpp#L2421.


================
Comment at: test/tools/yaml2obj/elf-hash-section.yaml:65
+
+## Check we can't use "Content" and "Bucket" tags together.
+
----------------
jhenderson wrote:
> What about "Content" and "Chain"?
Seems I've forgot about this combination. Added a test.


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


================
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);
----------------
jhenderson wrote:
> `4` -> `sizeof (Elf32_Word)`?
See my comment about this above.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:663
+
+  assert(Cur && "entries were not read correctly");
+  return S.release();
----------------
MaskRay wrote:
> if LLVM_ENABLE_ABI_BREAKING_CHECKS && !LLVM_ENABLE_ASSERTIONS, this will fail.
> 
> Add `!Cur` outside.
I also used `llvm_unreachable`. It should be impossible to have any errors here.


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

https://reviews.llvm.org/D68085