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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+    Type:    SHT_RELR
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
jhenderson wrote:
> 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)
> 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)

It is not about a dumping probably? We are testing yaml2obj here.
I've added four "## Set an arbitrary flag to demonstrate it is not dropped." comments around.
Does it look OK for you?


================
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
----------------
jhenderson wrote:
> 64-bits -> 64-bit
> 
> (a target is described as being a 64/32-bit target - other places 64-bits would be correct).
(I hope I got it right)

Fixed here and in 3 more similar places in this test.
I've kept other comments, particularry one in `obj2yaml/relr-section.yaml`
("Test how we dump SHT_RELR sections for 32 and 64-bits targets.") unchanged.


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

https://reviews.llvm.org/D71872