[PATCH] D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Generally looks fine to me. Is there testing for the obj2yaml side of things? It feels like there needs to be testing there for writing the section header table in the right place in the YAML (with Offset if needed), but I don't see that yet.



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:197
+    BBAddrMap,
+    // Special chunks.
+    SpecialChunksStart,
----------------
Maybe add a new line above this comment to clearly split the two varieties.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:207-208
 
-  Chunk(ChunkKind K) : Kind(K) {}
+  // Usually sections/special chunks are not created implicitly, but loaded from
+  // YAML. When they are, this flag is used to signal about that.
+  bool IsImplicit;
----------------
I feel like this reads a bit better.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:294
+
+  size_t getHeadersNum(size_t SectionsNum) const {
+    if (IsImplicit)
----------------
`getNumHeaders` or maybe simply` size.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:715
+        return *S;
+    llvm_unreachable("the section header table chunk must always present");
+  }
----------------



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:338-340
   // We add a technical suffix for each unnamed section/fill. It does not affect
   // the output, but allows us to map them by name in the code and report better
   // error messages.
----------------
I think this comment now best belongs with the second `if` inside the loop, below the section header table bit, as it isn't relevant ot the section header table stuff.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:391
+    // When the section header table is explicitly defined at the end of the
+    // sections list, it is reasonable to assume that user wants to reorder
+    // section headers, but still wants to place the section header table after
----------------



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:405
+    Doc.Chunks.push_back(
+        std::make_unique<ELFYAML::SectionHeaderTable>(true /*IsImplicit*/));
 }
----------------



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:712
+      uint64_t Size = S->getHeadersNum(SHeaders.size()) * sizeof(Elf_Shdr);
+      CBA.writeZeros(Size);
+      LocationCounter += Size;
----------------
It probably deserves a comment here saying something about the section header information isn't all known yet, so fill with zeroes as a placeholder.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1840
+  const ELFYAML::SectionHeaderTable &SectionHeaders = Doc.getSectionHeaderTable();
+  if (!SectionHeaders.IsImplicit) {
+    if (SectionHeaders.Excluded)
----------------
Do you actually need the `IsImplicit` check here? The default values of NoHeaders and Excluded should be such that the ifs inside don't trigger anyway.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1972
+  const ELFYAML::SectionHeaderTable &SHT = Doc.getSectionHeaderTable();
+  if (SHT.Offset)
+    CBA.updateDataAt(*SHT.Offset, SHeaders.data(),
----------------
I think it would be more self-documenting if this was `if(!SHT.NoHeaders)`. If I understand it correctly, SHT.Offset will always be set when NoHeaders is false. If NoHeaders is false, specifying Offset explicitly doesn't make sense, and is prohibited.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1325
+  if (TypeStr == "Fill") {
+    assert(!IO.outputting()); // We don't dump fills currently;
+    Section.reset(new ELFYAML::Fill());
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers.yaml:339
+
+## Check we can place the section header table somewhere to the middle of sections list.
+
----------------



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

https://reviews.llvm.org/D95140