[PATCH] D71872: [yaml2obj/obj2yaml] - Add support for SHT_RELR sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:444
 
+struct RelocationOffsetsSection : Section {
+  Optional<std::vector<llvm::yaml::Hex64>> Entries;
----------------
grimar wrote:
> MaskRay wrote:
> > Why not `RelrSection`? Introducing a new term `RelocationOffsets` may be more confusing.
> I've renamed to `RelrSection`. I guess we might want to rename `RelocationSection` to `RelSection` too for consistency.
`RelocationSection` refers to both SHT_REL and SHT_RELA sections, right? Renaming to `RelSection` implies it's just for the former, to me.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:113
   typedef typename ELFT::Dyn Elf_Dyn;
+  using uintX_t = typename ELFT::uint;
 
----------------
Maybe you should just use typedef for consistency with the code above. I actually would find this harder to read as is if I read the whole block at once.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1455-1456
 
+  if (const auto *ROS = dyn_cast<ELFYAML::RelrSection>(C.get())) {
+    if (ROS->Entries && ROS->Content)
+      return "\"Entries\" and \"Content\" can't be used together";
----------------
`ROS` -> `RS`

(what does the 'O' stand for...?)


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:4
+## Test how we create SHT_RELR sections for 64-bits LE targets.
+## Test that sh_entsize is 8.
+# RUN: yaml2obj --docnum=1 %s -o %t.le64
----------------
I don't think this comment line here is necessary. It's implicit, since you actually check it below. (Why not also say, "that the sh_link and sh_info is 0"?)


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+    Type:    SHT_RELR
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
Is this important to the test?


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:38
+
+## Test how we create SHT_RELR sections for 64-bits BE targets.
+# RUN: yaml2obj --docnum=2 %s -o %t.be64
----------------
Perhaps worth changing this comment to say "Test that the content of SHT_RELR sections for 64-bits BE targets is correct".

I might also avoid abbreviating LE/BE in the comments (i.e. use "little endian" and "big endian").


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:61
+
+## Test how we create SHT_RELR sections for 32-bit LE targets.
+## Test that sh_entsize is 4.
----------------
Same comments as above for the 32-bit cases.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:173
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0xAABBCCDDEEFF0011 ]
+
----------------
Maybe worth making this explicitly 1 greater than the max (and adding a similar test for the 32-bit max case).


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:748
+  } else {
+    // Ignore. We are going to dump the data as a raw "Content" below.
+    consumeError(Relrs.takeError());
----------------
I'd write "We are going to dump the data as raw content below."


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

https://reviews.llvm.org/D71872