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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:444
 
+struct RelocationOffsetsSection : Section {
+  Optional<std::vector<llvm::yaml::Hex64>> Entries;
----------------
Why not `RelrSection`? Introducing a new term `RelocationOffsets` may be more confusing.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1106
+  commonSectionMapping(IO, Section);
+  IO.mapOptional("OffsetsAndBitmaps", Section.OffsetsAndBitmaps);
+  IO.mapOptional("Content", Section.Content);
----------------
grimar wrote:
> I wonder if we should call this field just "Values"?
I prefer `Entries`. 1) We already use `Entries` for some other sections. 2) `Entries` is the term used in the ELF spec proposal.

>   SHT_RELR: The section holds relative relocation entries without explicit
>             addends or info, such as type Elf32_Relr for the 32-bit class of
>             object files or type Elf64_Relr for the 64-bit class of object
>             files. An object file may have multiple relocation sections. See
>             ``Relocation'' below for details.


================
Comment at: llvm/test/tools/obj2yaml/relr-section.yaml:20
+# ELF64LE-NEXT:     EntSize: 0x0000000000000008
+# ELF64LE-NEXT:     Entries: [ 0x8877665544332211 ]
+
----------------
To make examples more plausible: use even numbers. Odd numbers not preceded by an even number are invalid.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:1
+## This is a test to test how we create SHT_RELR sections.
+
----------------
`This is a test to test` -> `Test`


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:35
+    Flags:   [ SHF_ALLOC ]
+## An arbitrary 32-bits values.
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
It seems the comments can be deleted.

The indentation of the continuation line is strange.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:750
+  } else {
+    consumeError(Relrs.takeError());
+  }
----------------
Why can the error be swallowed?


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

https://reviews.llvm.org/D71872