[PATCH] D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D53576#1274235, @Higuoxing wrote:

> Hi, I upload a new test. Did I miss something? Thanks a lot


The test is looking better now. I have a few comments about the sections you are putting in it, inline.



================
Comment at: test/tools/llvm-objdump/full-contents.test:24
+  - Name:            .bss
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
----------------
.bss sections are usually of type SHT_NOBITS, and as such shouldn't have contents. Is there a particular reason you have broken with convention here?


================
Comment at: test/tools/llvm-objdump/full-contents.test:33
+    AddressAlign:    0x0000000000000010
+    Content:         "66756c6c2d636f6e74656e747300000074657374"
+    Size:            32
----------------
Let's make the content of this section really small, say 4 bytes. Then use FileCheck to check that the dump contains the bytes expected. It doesn't need to contain valid strings or executable instructions for this test to be good.


================
Comment at: test/tools/llvm-objdump/full-contents.test:34
+    Content:         "66756c6c2d636f6e74656e747300000074657374"
+    Size:            32
+  - Name:            .user-defined
----------------
Make sure the section size matches the content size for our test.


================
Comment at: test/tools/llvm-objdump/full-contents.test:35
+    Size:            32
+  - Name:            .user-defined
+    Type:            SHT_PROGBITS
----------------
Same comments for .text. I'd also give it different content to the other section, so that you can show that each section has the correct content dumped.


================
Comment at: test/tools/llvm-objdump/full-contents.test:46-51
+Symbols:
+   Global:
+     - Name:     foo
+       Type:     STT_FUNC
+       Section:  .text
+       Value:    0x1004
----------------
I think you can probably get rid of this symbol. It doesn't add anything to this test.


https://reviews.llvm.org/D53576