This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Higuoxing on Oct 23 2018, 7:49 AM.

Diff Detail

Event Timeline

Higuoxing created this revision.Oct 23 2018, 7:49 AM
jhenderson added a subscriber: 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!

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 updated this revision to Diff 170673.Oct 23 2018, 9:18 AM

add simple test

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

    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.
  1. Assuming you are using yaml2obj, you will probably need to put comment markers at the start of each CHECK, like the RUN lines.
  2. You need to call FileCheck to actually run the CHECKs in your file.
  3. 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).
  4. "CHECK: " with no output after it does not do anything. If you are really interested in the blank line, you should use CHECK-EMPTY.
  5. 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.

Thanks a lot, I will re-write some tests

Higuoxing updated this revision to Diff 170888.EditedOct 24 2018, 7:42 AM

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

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
25

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

34

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.

35

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

36

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.

47–52

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

Higuoxing updated this revision to Diff 170904.Oct 24 2018, 8:35 AM

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

jhenderson accepted this revision.Oct 24 2018, 8:47 AM

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–123

Have you clang-formatted this area?

This revision is now accepted and ready to land.Oct 24 2018, 8:47 AM

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 this revision to Diff 170911.EditedOct 24 2018, 9:14 AM

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

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

Sure, will do, but not until tomorrow!

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

This revision was automatically updated to reflect the committed changes.