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

[Higuoxing]

Summary

See: https://bugs.llvm.org/show_bug.cgi?id=39404

[jhenderson]

It would be good if there were a test for both switches, but I don't see an existing explicit test for -s. Are you okay adding one?

P.S. Don't forget to add some reviewers to your reviews on Phabricator!

[Higuoxing]

In D53576#1272486, @jhenderson wrote:

It would be good if there were a test for both switches, but I don't see an existing explicit test for -s. Are you okay adding one?

P.S. Don't forget to add some reviewers to your reviews on Phabricator!

Of course, thanks a lot :)

[Higuoxing]

add simple test

[jhenderson]

Thanks for the test. It's a good start, but there are several issues with it:

1. 1. Don't rely on pre-canned binaries that are for other not-really-related tests. You would be better off using yaml2obj to create a test input, specific to this test. This makes it clearer what is important to the test, and leads to a less fragile test, because modifying the other test won't break this test. Take a look at test/tools/llvm-objcopy/basic-copy.test for a good example of how to create the test input at test-time.
2. Assuming you are using yaml2obj, you will probably need to put comment markers at the start of each CHECK, like the RUN lines.
3. You need to call FileCheck to actually run the CHECKs in your file.
4. You might want to pipe the output of the two llvm-objdump calls to a file, and then compare them to show that both options produce the same output (FileCheck will only check the things you check, and not the things you don't check).
5. "CHECK: " with no output after it does not do anything. If you are really interested in the blank line, you should use CHECK-EMPTY.
6. Use "CHECK-NEXT:" to check that the next line is actually on the next line of the output. "CHECK:" simply checks that the pattern is somewhere later in the file than teh previous one.

Here's an incomplete example of what you might want to do:

# RUN: yaml2obj %s > %t
# RUN: llvm-objdump --full-contents %t > %t.out1
# RUN: llvm-objdump -s %t > %t.out2
# RUN: cmp %t.out1 %t.out2
# RUN: FileCheck %s --input-file=%t.out1

# CHECK: some stuff
# CHECK-NEXT: some more stuff
## etc

!ELF
FileHeader:
## the rest of the yaml here.

[Higuoxing]

Thanks a lot, I will re-write some tests

[Higuoxing]

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

[jhenderson]

In 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.

test/tools/llvm-objdump/full-contents.test

24

.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?

33

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.

34

Make sure the section size matches the content size for our test.

35

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.

46–51

I think you can probably get rid of this symbol. It doesn't add anything to this test.

[Higuoxing]

Thanks, I am reading the ELF format these days. So, I may make some stupid mistakes, sorry for that .

[jhenderson]

LGTM.

Do you need me to commit it?

I have one inlined query about formatting. If you haven't clang-formatted the area, please do so before committing.

tools/llvm-objdump/llvm-objdump.cpp

119–124

Have you clang-formatted this area?

[Higuoxing]

In D53576#1274371, @jhenderson wrote:

LGTM.

Do you need me to commit it?

I have one inlined query about formatting. If you haven't clang-formatted the area, please do so before committing.

Thanks a lot,

The formatted one is

cl::opt<bool>
    llvm::SectionContents("full-contents",
                          cl::desc("Display the content of each section"));
static cl::alias SectionContentsShort("s",
                                      cl::desc("Alias for --full-contents"),
                                      cl::aliasopt(SectionContents));

but I format it as previous function DynamicRelocations

[Higuoxing]

Updated with clang-formatted codes, Please help me commit it, thanks a lot :)

[jhenderson]

In D53576#1274407, @Higuoxing wrote:

Updated with clang-formatted codes, Please help me commit it, thanks a lot :)

Sure, will do, but not until tomorrow!

[jhenderson]

Sorry for the delay, I was off sick. I will be committing this shortly.

test/tools/llvm-objdump/full-contents.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump --full-contents %t > %t.out1
+# RUN: llvm-objdump -s %t > %t.out2
+# RUN: cmp %t.out1 %t.out2
+# RUN: FileCheck %s --input-file=%t.out1
+
+# CHECK:       .bss
+# CHECK:       .text
+# CHECK:       .user-defined
+# CHECK:       .empty-section
+# CHECK-NEXT:  <skipping contents of bss section at [0000, 0020)>
+# CHECK:       .symtab
+# CHECK:       .strtab
+# CHECK:       .shstrtab
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "66756c6c2d636f6e74656e747300000074657374"
+    Size:            32
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "66756c6c2d636f6e74656e747300000074657374"
+    Size:            32
+  - Name:            .user-defined
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000010
+    Content:         "66756c6c2d636f6e74656e747300000074657374"
+    Size:            32
+  - Name:            .empty-section
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000010
+    Size:            32
+Symbols:
+   Global:
+     - Name:     foo
+       Type:     STT_FUNC
+       Section:  .text
+       Value:    0x1004

tools/llvm-objdump/llvm-objdump.cpp

 cl::opt<bool>
 llvm::DynamicRelocations("dynamic-reloc",
   cl::desc("Display the dynamic relocation entries in the file"));
 static cl::alias
 DynamicRelocationsd("R", cl::desc("Alias for --dynamic-reloc"),
              cl::aliasopt(DynamicRelocations));
 
 cl::opt<bool>
-llvm::SectionContents("s", cl::desc("Display the content of each section"));
+llvm::SectionContents("full-contents",
+  cl::desc("Display the content of each section"));
+static cl::alias
+SectionContentsShort("s", cl::desc("Alias for --full-contents"),
+             cl::aliasopt(SectionContents));
 
 cl::opt<bool>
 llvm::SymbolTable("t", cl::desc("Display the symbol table"));
 
 cl::opt<bool>
 llvm::ExportsTrie("exports-trie", cl::desc("Display mach-o exported symbols"));
 
 cl::opt<bool>
 llvm::Rebase("rebase", cl::desc("Display mach-o rebasing info"));