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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:113
   typedef typename ELFT::Dyn Elf_Dyn;
+  using uintX_t = typename ELFT::uint;
 
----------------
jhenderson wrote:
> 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.
OK. I think I had an oppsoite comments before (not from you). About switching to "using".
That is why I did it.
I think we can change the whole block to `using` at once later.


================
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";
----------------
jhenderson wrote:
> `ROS` -> `RS`
> 
> (what does the 'O' stand for...?)
RelOcationSection I guess... fixed.


================
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
----------------
jhenderson wrote:
> 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"?)
Mostly it was done because I set sh_entsize in the code of this patch.
I find your comment valid though, so I've removed this line.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+    Type:    SHT_RELR
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
jhenderson wrote:
> Is this important to the test?
Why not? It makes sence to show we do not drop a flag specified.
Indeed it is not a full test for this bit, but it tests something and is better than nothing.
(I do not feel we want to test much more than that atm).

Your comment was usefull for other tests below though, where I've removed the `Flags` field.


================
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
----------------
jhenderson wrote:
> 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").
Done.

FTR. I am not sure about LE/BE ->  "little endian" and "big endian" conversion,
because I think that LE/BE are common abbreviations.


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


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

https://reviews.llvm.org/D71872