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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/relr-section.yaml:1
+## Test how we dump SHT_RELR sections for 32 and 64-bits targets.
+
----------------
Please update this 64-bits -> 64-bit too.


================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+    Type:    SHT_RELR
+    Flags:   [ SHF_ALLOC ]
+    Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
grimar wrote:
> 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?
I'd change "it is not dropped" -> "flags are set when requested" perhaps. Otherwise, looks good.


================
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
----------------
grimar wrote:
> 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.
After thinking about it yesterday, I think I figured out the rule. When 32/64-bit is used as an adjective followed by a noun (e.g. "This is a 64-bit target"), it shouldn't have the 's', whereas when it is used without a following noun (e.g. "This target is 64-bits"), it should.

Aside from the obj2yaml/relr-section.yaml case (which should be updated), I couldn't find any other 64-bits usage in comments in this patch. Which were the others you were referring to?


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

https://reviews.llvm.org/D71872