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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:113
   typedef typename ELFT::Dyn Elf_Dyn;
+  using uintX_t = typename ELFT::uint;
 
----------------
grimar wrote:
> 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.
Right. I agree that in general `using` is easier to read, so I'd certainly support switching over wholesale in this area at another point.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+    Type:    SHT_RELR
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
grimar wrote:
> 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.
> Indeed it is not a full test for this bit, but it tests something and is better than nothing.
Okay. I think it needs a comment to explain why it is tested then. Perhaps something along the lines of "Show that relr section dumping maintains common section properties such as flags." (there may be a clearer or more concise phrasing)


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:3
+
+## Test that the content of SHT_RELR sections for 64-bits little endian targets is correct.
+# RUN: yaml2obj --docnum=1 %s -o %t.le64
----------------
64-bits -> 64-bit

(a target is described as being a 64/32-bit target - other places 64-bits would be correct).


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

https://reviews.llvm.org/D71872