[PATCH] D70316: [llvm-readobj] Allow printing of the watermark note section proposed in D66426

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not looked at the code here yet. I'll do that once there's a consensus on D66426 <https://reviews.llvm.org/D66426>.



================
Comment at: llvm/test/tools/llvm-readobj/elf-watermark.test:1
+## Test that we can calculate and print the loadable segment watermark for an
+## elf file and ensure this watermark matches that computed by lld.
----------------
Assembly tests should have a '.s' extension. Also, since this is using X86_64 assembly, it needs a `REQUIRES: x86`. Finally, please rebase this change (there have been significant changes to the test directory structure recently).


================
Comment at: llvm/test/tools/llvm-readobj/elf-watermark.test:6
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld --watermark %t -o %t.watermark
+# RUN: llvm-readelf -x .note.llvm.watermark %t.watermark | FileCheck --strict-whitespace -check-prefix=DUMP %s
----------------
LLD can't be used in an llvm-readobj test I'm afraid (it is not required to build check-llvm). You'll need a different way to setup this test somehow, or possibly this should be moved into the LLD testing if no sensible way can be come up with. One option might be to pre-calculate the watermark for a given set of content and use yaml2obj to generate the file with that watermark in it.


================
Comment at: llvm/test/tools/llvm-readobj/elf-watermark.test:7
+# RUN: ld.lld --watermark %t -o %t.watermark
+# RUN: llvm-readelf -x .note.llvm.watermark %t.watermark | FileCheck --strict-whitespace -check-prefix=DUMP %s
+# RUN: llvm-readobj --compute-watermark %t.watermark | FileCheck --strict-whitespace -check-prefix=COMPUTE %s
----------------
-check-prefix -> --check-prefix (also below)


================
Comment at: llvm/test/tools/llvm-readobj/elf-watermark.test:10
+
+# DUMP: Hex dump of section '.note.llvm.watermark':
+# DUMP-NEXT: 05000000 0c000000 04000000 4c4c564d ............LLVM
----------------
Minor nit: add extra spacing to make the value being checked line up with the following lines.


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:1
+## Test tools are able to dump the LLVM Watermark note section. Also test the
+## output when the type field is incorrect.
----------------
This test's name is `note-llvm.test`, but the test is specifically about the watermark note?


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:3-4
+## output when the type field is incorrect.
+## Show also that if the watermark is mapped to a PT_NOTE segment that the
+## watermark can be dumped even if llvm-objcopy --strip-sections has been used.
+
----------------
This part feels like it's more generically "notes can be dumped after --strip-sections has been used". Why is the watermark here special?


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:6
+
+# RUN: yaml2obj -docnum=1 %s > %t1.o
+# RUN: llvm-readobj --notes %t1.o | FileCheck %s --check-prefix=LLVM
----------------
General consensus is to use -o rather than shell redirection in new tests.

Also `--docnum`.


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:41
+    Content:      050000000c000000040000004c4c564d000000000100000045e69f81a671eaac
+
+
----------------
Nit: too many blank lines


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:43
+
+# RUN: yaml2obj -docnum=2 %s > %t2.o
+# RUN: llvm-readobj --notes %t2.o | FileCheck %s --check-prefix=LLVM_UNKNOWNTYPE
----------------
Same as above.


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:76
+
+# RUN: yaml2obj -docnum=3 %s > %t3.o
+# RUN: llvm-objcopy --strip-sections %t3.o %t3.stripped.o
----------------
Same as above.

The comment related to this section should be moved to here.


================
Comment at: llvm/test/tools/llvm-readobj/note-llvm.test:114-115
+# GNU_STRIPPED:      Displaying notes found at file offset 0x00000040 with length 0x00000020:
+# GNU_STRIPPED-NEXT: Owner                Data size    Description
+# GNU_STRIPPED-NEXT: LLVM                 0x0000000c       NT_LLVM_WATERMARK (Loadable sections watermark)
+# GNU_STRIPPED-NEXT:    LLVM Watermark: beeeeeeeeeeeeeef Version: 0x01000000
----------------
The spacing looks all off here. Either remove excessive spacing so that it all lines up, or add --strict-whitespace to show that you're testing the latter.


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

https://reviews.llvm.org/D70316