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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:444
 
+struct RelocationOffsetsSection : Section {
+  Optional<std::vector<llvm::yaml::Hex64>> Entries;
----------------
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.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1106
+  commonSectionMapping(IO, Section);
+  IO.mapOptional("OffsetsAndBitmaps", Section.OffsetsAndBitmaps);
+  IO.mapOptional("Content", Section.Content);
----------------
MaskRay wrote:
> 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.
Yeah. I initially called this field "OffsetsAndBitmaps", suggested "Values", but then renamed to "Entries" because of the same reasons you mention.


================
Comment at: llvm/test/tools/obj2yaml/relr-section.yaml:20
+# ELF64LE-NEXT:     EntSize: 0x0000000000000008
+# ELF64LE-NEXT:     Entries: [ 0x8877665544332211 ]
+
----------------
MaskRay wrote:
> To make examples more plausible: use even numbers. Odd numbers not preceded by an even number are invalid.
> Odd numbers not preceded by an even number are invalid.
Sure, but in obj2yaml we do not need to care about such things. Moreover, we probably want to check
exactly this cases, because one of the tasks of `obj2yaml` is to produce the output from any given object.
Object can be valid or can be not. We only want to show that we use "Entries" when `data size % 64(32) == 0`.

The same applies for yaml2obj probably.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:750
+  } else {
+    consumeError(Relrs.takeError());
+  }
----------------
MaskRay wrote:
> Why can the error be swallowed?
If we can't read the content as array of `ELF_Relr`, we fall back to read it as an array of `uint8_t`s.
We do not need to trigger any errors, because while we are able to dump it at least with use of "Content",
we are fine. If we can't do that, the appropriate error will be reported below.

I could just read the whole content as array from the begining and then write checks, like "if (Size % sizeof(uintX_t) != 0)",
and then read values manually, but I supposed the code looks a bit simpler in this way.


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

https://reviews.llvm.org/D71872