[llvm-size] print a blank line between archieve members when using sysv format

[TH3CHARLie]

Summary

This patch is related to https://bugs.llvm.org/show_bug.cgi?id=42967 and it fixes llvm-size's sysv format output by adding a blank line between archieve members

[Jim]

The testcases tools/llvm-size/archive.test, tools/llvm-size/multiple-inputs.test and tools/llvm-size/radix.test
fail due to this patch. Could you update this testcases? You can try to execute command "ninja check-all -j32".

llvm/tools/llvm-size/llvm-size.cpp

449–450

Put "<< "\n"" in the new line look better.

outs() << format... 
       << "\n";

[TH3CHARLie]

update testcases

[TH3CHARLie]

In D71957#1798590, @Jim wrote:

The testcases tools/llvm-size/archive.test, tools/llvm-size/multiple-inputs.test and tools/llvm-size/radix.test
fail due to this patch. Could you update this testcases? You can try to execute command "ninja check-all -j32".

Hi, thanks for the reivew and I have updated it with a new patch. However, I'm new to LLVM's review system, I wonder where can I check the status of the testcases, it will be grateful if you can point it out to me.

[Jim]

llvm/tools/llvm-size/llvm-size.cpp

449

It should be origin one before any of your changes.

[TH3CHARLie]

revert to origin

[Jim]

There shows "Context not available."
Please follow https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface to upload patch.

[TH3CHARLie]

fix context

[Jim]

LGTM.
But I have tried gnu size, and I found that has two blank lines between archive members.
Here only one blank line.
What's your opinion?

[TH3CHARLie]

In D71957#1799537, @Jim wrote:

LGTM.
But I have tried gnu size, and I found that has two blank lines between archive members.
Here only one blank line.
What's your opinion?

If strictly following to the issue's(recall from https://bugs.llvm.org/show_bug.cgi?id=42967) description, adding one blank line would be enough. But indeed this looks inconsistent with GNU's size in sysv format:

1. GNU's size print two blank lines between archive members, this PR prints one
2. At end of the file, GNU's size prints no newline(but since last archive member prints two blank lines), llvm-size prints an additional newline at end of file(see line 843-845).

Thus, I think it is better to adjust this PR so llvm-size's behavior is identical with GNU's

[TH3CHARLie]

I update the patch, now llvm-size's behavior is more(if not 100%) similar to GNU's size:

1. two newlines after archive member
2. no newline when the archive is empty

[Jim]

LGTM.

[MaskRay]

llvm/tools/llvm-size/llvm-size.cpp

579

Delete else and add a new line as you added return above.

[MaskRay]

llvm/test/tools/llvm-size/archive.test

51

GNU size prints 2 blank lines.

[TH3CHARLie]

delete else and add newline

[TH3CHARLie]

llvm/test/tools/llvm-size/archive.test

51

I believe this is necessary, see radix.test's case 4-6

[TH3CHARLie]

@MaskRay Hi, could you please review the newest update and commit it if you feel OK about it since looks like I have no commit access

[MaskRay]

llvm/test/tools/llvm-size/archive.test

51

--format=sysv prints exactly two blank lines between two blocks. Your patch currently prints 3.

After you fix this, you may need to provide git commit --amend --author "what-is-your-name <what-is@your-email>"

[TH3CHARLie]

llvm/test/tools/llvm-size/archive.test

51

As you can tell from the patch, there was a SYSV-3-EMPTY in the testcase before this patch. The original code prints no new lines between blocks and to match the newly added 2 newlines, 2 corresponding SYSV-3-EMPTY is needed here.

If we remove a SYSV-3-EMPTY here, error will be generated:

bash
/home/yxd/llvm-project/llvm/test/tools/llvm-size/archive.test:52:16: error: SYSV-3-NEXT: is not on the line after the previous match
# SYSV-3-NEXT: archive.test.tmp1 (ex [[ARCHIVE2]]):
               ^
<stdin>:10:1: note: 'next' match was here
archive.test.tmp1 (ex /home/yxd/llvm-project/build/test/tools/llvm-size/Output/archive.test.tmp2.a):
^
<stdin>:8:1: note: previous match ended here

^
<stdin>:9:1: note: non-matching line after previous match is here

Although I'm not sure about why there was a SYSV-3-EMPTY here before adding any newlines, it is required here to add the two SYSV-3-EMPTY to pass the test. BTW, I have also verified with custom testcases.

[Jim]

llvm/test/tools/llvm-size/archive.test

51

This is non-archives (single object file).

llvm/tools/llvm-size/llvm-size.cpp

826

Fix here. It prints 3 blank lines. 2 blank lines is from printObjectSectionSizes and 1 blank line from

// System V adds an extra newline at the end of each file.
  if (OutputFormat == sysv)
    outs() << "\n";

[TH3CHARLie]

The extra newline originally printed is now removed since GNU size print two newlines for single object files

[jhenderson]

LGTM. Please make sure @Jim is happy with this too.

llvm/tools/llvm-size/llvm-size.cpp

449–450

I think this is correct, but to be sure if you haven't already, run git-clang-format on the modified part to verify your formatting choices match the official style (see https://llvm.org/docs/GettingStarted.html#sending-patches).

[Jim]

llvm/tools/llvm-size/llvm-size.cpp

579

I think this change is not needed. If you delete line 842-844.

[TH3CHARLie]

restore if-else logic

[TH3CHARLie]

llvm/tools/llvm-size/llvm-size.cpp

449–450

Thanks for the heads up and I've run the clang-format which put the "\n\n" into a newline

[TH3CHARLie]

@Jim Thanks for the review! I wonder is there anything else I have to do before committing the patch? I've used git commit --amend to add author and e-mail and what should I do next. And seems like I have no commit access according to https://llvm.org/docs/Phabricator.html#committing-a-change

[Jim]

@TH3CHARLie
I can commit this for you.
git commit --amend --author="What-is-your-name <What-is-your-email-address>" ?
If you are interested in obtaining commit right. please refer to https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[TH3CHARLie]

In D71957#1802393, @Jim wrote:

@TH3CHARLie
I can commit this for you.
git commit --amend --author="What-is-your-name <What-is-your-email-address>" ?
If you are interested in obtaining commit right. please refer to https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Please commit this for me
git commit --amend --author="TH3CHARLie <[e-mail]>"
And I am interested in obtaining commit right for future revisions, thank you and I will definitely take a look

llvm/test/tools/llvm-size/archive.test

 
 # SYSV-3:      [[FILE1]]  :
 # SYSV-3-NEXT: section     size   addr
 # SYSV-3-NEXT: .text          1      4
 # SYSV-3-NEXT: .data          2      2
 # SYSV-3-NEXT: .bss           4      1
 # SYSV-3-NEXT: Total          7
 # SYSV-3-EMPTY:
+# SYSV-3-EMPTY:
 # SYSV-3-NEXT: archive.test.tmp1   (ex [[ARCHIVE2]]):
 # SYSV-3-NEXT: section     size   addr
 # SYSV-3-NEXT: .text          1      4
 # SYSV-3-NEXT: .data          2      2
 # SYSV-3-NEXT: .bss           4      1
 # SYSV-3-NEXT: Total          7
 # SYSV-3-EMPTY:
+# SYSV-3-EMPTY:
 # SYSV-3-NEXT: [[FILE2]]  :
 # SYSV-3-NEXT: section     size   addr
 # SYSV-3-NEXT: .text          8      32
 # SYSV-3-NEXT: .data         16      16
 # SYSV-3-NEXT: .bss          32      8
 # SYSV-3-NEXT: Total         56
 # SYSV-3-EMPTY:
 
 # SYSV-1:      archive.test.tmp1   (ex [[ARCHIVE]]):
 # SYSV-1-NEXT: section     size   addr
 # SYSV-1-NEXT: .text          1      4
 # SYSV-1-NEXT: .data          2      2
 # SYSV-1-NEXT: .bss           4      1
 # SYSV-1-NEXT: Total          7
+# SYSV-1-EMPTY:
 
 # SYSV-2-NEXT: archive.test.tmp2   (ex [[ARCHIVE]]):
 # SYSV-2-NEXT: section     size   addr
 # SYSV-2-NEXT: .text          8      32
 # SYSV-2-NEXT: .data         16      16
 # SYSV-2-NEXT: .bss          32      8
 # SYSV-2-NEXT: Total         56
 # SYSV-1-NOT:{{.}}

llvm/test/tools/llvm-size/multiple-inputs.test

 
 # SYSV:      [[FILE1]]  :
 # SYSV-NEXT: section     size   addr
 # SYSV-NEXT: .text          1      4
 # SYSV-NEXT: .data          2      2
 # SYSV-NEXT: .bss           4      1
 # SYSV-NEXT: Total          7
 # SYSV-EMPTY:
+# SYSV-EMPTY:
 # SYSV-NEXT: [[FILE2]]  :
 # SYSV-NEXT: section     size   addr
 # SYSV-NEXT: .text          8     32
 # SYSV-NEXT: .data         16     16
 # SYSV-NEXT: .bss          32      8
 # SYSV-NEXT: Total         56
 # SYSV-NOT:{{.}}
 

llvm/test/tools/llvm-size/radix.test

 
 # OCT-SYSV:      [[FILE1]]  :
 # OCT-SYSV-NEXT: section     size   addr
 # OCT-SYSV-NEXT: .text         01     04
 # OCT-SYSV-NEXT: .data         02     02
 # OCT-SYSV-NEXT: .bss          04     01
 # OCT-SYSV-NEXT: Total         07
 # OCT-SYSV-EMPTY:
+# OCT-SYSV-EMPTY:
 # OCT-SYSV-NEXT: [[FILE2]]  :
 # OCT-SYSV-NEXT: section     size   addr
 # OCT-SYSV-NEXT: .blob        010    040
 # OCT-SYSV-NEXT: .blab        020    020
 # OCT-SYSV-NEXT: .blib        040    010
 # OCT-SYSV-NEXT: Total        070
 
 ## Case 5: Decimal radix (sysv).
 # RUN: llvm-size %t1.o %t2.o -A --radix=10 \
 # RUN:   | FileCheck %s --check-prefix=DEC-SYSV -DFILE1=%t1.o -DFILE2=%t2.o
 # RUN: llvm-size %t1.o %t2.o -A -d \
 # RUN:   | FileCheck %s --check-prefix=DEC-SYSV -DFILE1=%t1.o -DFILE2=%t2.o
 
 # DEC-SYSV:      [[FILE1]]  :
 # DEC-SYSV-NEXT: section     size   addr
 # DEC-SYSV-NEXT: .text          1      4
 # DEC-SYSV-NEXT: .data          2      2
 # DEC-SYSV-NEXT: .bss           4      1
 # DEC-SYSV-NEXT: Total          7
 # DEC-SYSV-EMPTY:
+# DEC-SYSV-EMPTY:
 # DEC-SYSV-NEXT: [[FILE2]]  :
 # DEC-SYSV-NEXT: section     size   addr
 # DEC-SYSV-NEXT: .blob          8     32
 # DEC-SYSV-NEXT: .blab         16     16
 # DEC-SYSV-NEXT: .blib         32      8
 # DEC-SYSV-NEXT: Total         56
 
 ## Case 6: Hexadecimal radix (sysv).
 # RUN: llvm-size %t1.o %t2.o -A --radix=16 \
 # RUN:   | FileCheck %s --check-prefix=HEX-SYSV -DFILE1=%t1.o -DFILE2=%t2.o
 # RUN: llvm-size %t1.o %t2.o -A -x \
 # RUN:   | FileCheck %s --check-prefix=HEX-SYSV -DFILE1=%t1.o -DFILE2=%t2.o
 
 # HEX-SYSV:      [[FILE1]]  :
 # HEX-SYSV-NEXT: section     size   addr
 # HEX-SYSV-NEXT: .text        0x1    0x4
 # HEX-SYSV-NEXT: .data        0x2    0x2
 # HEX-SYSV-NEXT: .bss         0x4    0x1
 # HEX-SYSV-NEXT: Total        0x7
 # HEX-SYSV-EMPTY:
+# HEX-SYSV-EMPTY:
 # HEX-SYSV-NEXT: [[FILE2]]  :
 # HEX-SYSV-NEXT: section     size   addr
 # HEX-SYSV-NEXT: .blob        0x8   0x20
 # HEX-SYSV-NEXT: .blab       0x10   0x10
 # HEX-SYSV-NEXT: .blib       0x20    0x8
 # HEX-SYSV-NEXT: Total       0x38
 
 ## Case 7: Default Berkeley is decimal.

llvm/tools/llvm-size/llvm-size.cpp

if (ELFCommons) {
                        CommonSize, static_cast<uint64_t>(0));
     }
 
     // Print total.
     fmtbuf.clear();
     fmt << "%-" << max_name_len << "s "
         << "%#" << max_size_len << radix_fmt << "\n";
     outs() << format(fmt.str().c_str(), static_cast<const char *>("Total"),
-                     total) << "\n";
+                     total)
+           << "\n";
   } else {
     // The Berkeley format does not display individual section sizes. It
     // displays the cumulative size for each section type.
     uint64_t total_text = 0;
     uint64_t total_data = 0;
     uint64_t total_bss = 0;
 
     // Make one pass over the section table to calculate sizes.
for (auto &C : a->children(Err)) {
             outs() << a->getFileName() << "(" << o->getFileName() << ")\n";
           else
             outs() << o->getFileName() << " (ex " << a->getFileName() << ")\n";
         }
       }
     }
     if (Err)
       error(std::move(Err), a->getFileName());
   } else if (MachOUniversalBinary *UB =
                  dyn_cast<MachOUniversalBinary>(&Bin)) {
     // If we have a list of architecture flags specified dump only those.
     if (!ArchAll && !ArchFlags.empty()) {
       // Look for a slice in the universal binary that matches each ArchFlag.
       bool ArchFound;
       for (unsigned i = 0; i < ArchFlags.size(); ++i) {
         ArchFound = false;
         for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
       } else {
         consumeError(AOrErr.takeError());
         error("mach-o universal file for architecture " +
                   StringRef(I->getArchFlagName()) +
                   " is not a mach-o file or an archive file",
               file);
       }
     }
   } else if (ObjectFile *o = dyn_cast<ObjectFile>(&Bin)) {
     if (!checkMachOAndArchFlags(o, file))
       return;
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(o);
     if (OutputFormat == sysv)
       outs() << o->getFileName() << "  :\n";
     else if (MachO && OutputFormat == darwin && MoreThanOneFile)
       outs() << o->getFileName() << ":\n";
     printObjectSectionSizes(o);