[llvm-objdump] Adjust spacing and field width for --section-headers

[rupprecht]

Summary

• Expand the "Name" column past 13 characters when any of the section names are longer. Current behavior is a staggard output instead of a nice table if a single name is longer.
• Only print the required number of hex chars for addresses (i.e. 8 characters for 32-bit, 16 characters for 64-bit)
• Fix trailing spaces

[grimar]

llvm/test/tools/llvm-objdump/section-headers-address-width.test

10 ↗

(On Diff #224163)

I wonder, should we be able to shrink it too?

Something like:
Len = max length among all sections.

Instead of a current (if I understand it right)
Len = max(max length ..., 13)

37 ↗

(On Diff #224163)

nit: We are often reducing the identation to a single space after the longest tag. Just like you have for FileHeader
(here and everywhere below)

38 ↗

(On Diff #224163)

nit: you can remove '...'
(here and everywhere below)

llvm/test/tools/llvm-objdump/section-headers-name-width.test

2 ↗

(On Diff #224163)

nit: missing empty line after comment.

4 ↗

(On Diff #224163)

You are not using --show-lma it seems? I.e. I am not sure why do you need this test.

16 ↗

(On Diff #224163)

nit: We often add a short comment for each test in addition to a file comment.
I.e. you could say something like "here we test that name with the length 14 expands ..."

llvm/test/tools/llvm-objdump/section-headers-spacing.test

1 ↗

(On Diff #224163)

What do you think about combining these tests you have here into one that
could use yaml2obj --docnum=X and check spacing, formatting etc in one place?
(I am not sure it if it is usefull to have 3 different test files?)

llvm/tools/llvm-objdump/llvm-objdump.cpp

1653

getMaxSectionNameWidth?

1694

May be I'd try to avoid using an additional vector and algorithm here and just:

std::string Type = Section.isText() ? "TEXT" : "";
if (Section.isData())
  Type += Type.empty() ? "DATA" : " DATA";
if (Section.isBSS())
  Type += Type.empty() ? "BSS" : " BSS";

[rupprecht]

llvm/test/tools/llvm-objdump/section-headers-address-width.test

10 ↗

(On Diff #224163)

13 is chosen to make the output match GNU objdump. Having a cutoff (not necessarily 13, but any number around there) also makes the formatting stable for objects with regular-length section names.

llvm/test/tools/llvm-objdump/section-headers-name-width.test

4 ↗

(On Diff #224163)

Added a comment -- the reason is just that it goes down a different code path, e.g. would catch a bad implementation like:

if (ShowLMA)
  outs() << "Name    " << ... // Oops, didn't use left_justify
else
  outs() << left_justify("Name", NameWidth) << ...

llvm/test/tools/llvm-objdump/section-headers-spacing.test

1 ↗

(On Diff #224163)

I started out with one test file, but found it to be a collection of somewhat unrelated things -- e.g. name column width and 32 vs 64 bit column widths are different features. So I think it's better to have more focused test files. It's a slightly personal preference though.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1694

I think the lack of using an algorithm is why there was odd trailing whitespace before, although I agree it would be great if this could be more succinct...

[rupprecht]

• Remove yaml ... separators
• Whitespace changes
• Add more test comments
• Rename to getMaxSectionNameWidth
• Fix yaml spacing
• Remove some unnecessary yaml fields

[grimar]

llvm/test/tools/llvm-objdump/section-headers-spacing.test

1 ↗

(On Diff #224163)

We often combine tests by a feature. I.e. test that checks the "-h" output might contain everything related to "-h" at once. Sometimes we do a split to make a test that contain only a error/warnings checks (if there are too many of them) or a particular set of tests that are very different.

It is not a huge problem, but might be interesting what others think about this too though.
(to summarize my position: I'd prefer to combine them to reduce the number of tests, but it is not critical and is OK as is probably).

llvm/tools/llvm-objdump/llvm-objdump.cpp

1694

But does my version has a trailing whitespace issue? I think no.

[rupprecht]

• Merge tests
• Avoid llvm::join() call

llvm/test/tools/llvm-objdump/section-headers-spacing.test

1 ↗

(On Diff #224163)

I don't have that strong of preference either, so merged. My only concern is now it's on the larger end of test sizes (7th of all objdump tests per find llvm/test/tools/llvm-objdump/ -name '*.test' | xargs wc -l | sort -nr | tail +2 | head -10), but it's still not excessively long.

[grimar]

LGTM

[jhenderson]

llvm/test/tools/llvm-objdump/section-headers.test

8

No need to change this, but rather than using {{^}} and {{$}} you could have used --match-full-lines instead. Combined with --strict-whitespace, this will ensure that the line checked matches exactly the specified string. The downside is the lack of a space after the FileCheck pattern name, but you can solve that with additional spacing:

#      WHITESPACE:Sections:
# WHITESPACE-NEXT:Idx Name

or (I think, though not tested):

# WHITESPACE     :Sections:
# WHITESPACE-NEXT:Idx Name

lld/test/ELF/got32-i386.s

  nop
 
 _start:
  movl foo@GOT, %ebx
 
 ## 73728 == 0x12000 == ADDR(.got)
 # CHECK: Sections:
 # CHECK:  Name Size     VMA
-# CHECK:  .got 00000004 00000000004020fc
+# CHECK:  .got 00000004 004020fc
 # CHECK:       _start:
 # CHECK-NEXT:   4010f5: 8b 1d {{.*}}  movl 4202748, %ebx
 
 # RUN: not ld.lld %t.o -o %t -pie 2>&1 | FileCheck %s --check-prefix=ERR
 # ERR: error: symbol 'foo' cannot be preempted; recompile with -fPIE

lld/test/ELF/got32x-i386.s

 ## register, it should be calculated as G + A. Using without base register
 ## is only allowed for non-PIC code.
 ##
 # RUN: ld.lld %S/Inputs/i386-got32x-baseless.elf -o %t1
 # RUN: llvm-objdump -section-headers -d %t1 | FileCheck %s
 
 ## 73728 == 0x12000 == ADDR(.got)
 # CHECK: Sections:
-# CHECK:  Name Size     VMA
-# CHECK:  .got.plt 0000000c 0000000000403134
+# CHECK:  Name     Size     VMA
+# CHECK:  .got.plt 0000000c 00403134
 # CHECK:       _start:
 # CHECK-NEXT:   401115: 8b 05 {{.*}} movl 4202800, %eax
 # CHECK-NEXT:   40111b: 8b 1d {{.*}} movl 4202800, %ebx
 # CHECK-NEXT:   401121: 8b 80 {{.*}} movl -4100(%eax), %eax
 # CHECK-NEXT:   401127: 8b 83 {{.*}} movl -4100(%ebx), %eax
 
 # RUN: not ld.lld %S/Inputs/i386-got32x-baseless.elf -o %t1 -pie 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=ERR
 # ERR-COUNT-2: error: symbol 'foo' cannot be preempted; recompile with -fPIE

llvm/test/tools/llvm-objdump/section-headers.test

+## Test various details of llvm-objdump -h (--section-headers).
+
+## Check leading and trailing whitespace for full lines.
+
+# RUN: yaml2obj %s --docnum=1 -o %t-whitespace.o
+# RUN: llvm-objdump -h --show-lma %t-whitespace.o \
+# RUN:   | FileCheck %s --check-prefix=WHITESPACE --strict-whitespace
+# RUN: llvm-objdump -h %t-whitespace.o \
+# RUN:   | FileCheck %s --check-prefix=WHITESPACE-NO-LMA --strict-whitespace
+
+# WHITESPACE:      {{^}}Sections:{{$}}
+# WHITESPACE-NEXT: {{^}}Idx Name          Size     VMA              LMA              Type{{$}}
+# WHITESPACE-NEXT: {{^}}  0               00000000 0000000000000000 0000000000000000 {{$}}
+# WHITESPACE-NEXT: {{^}}  1 .text         00000000 0000000000000000 0000000000000000 TEXT{{$}}
+# WHITESPACE-NEXT: {{^}}  2 .data         00000000 0000000000000000 0000000000000000 DATA{{$}}
+# WHITESPACE-NEXT: {{^}}  3 .bss          00000000 0000000000000000 0000000000000000 BSS{{$}}
+# WHITESPACE-NEXT: {{^}}  4 .other        00000000 0000000000000000 0000000000000000 {{$}}
+
+# WHITESPACE-NO-LMA:      {{^}}Sections:{{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}Idx Name          Size     VMA              Type{{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}  0               00000000 0000000000000000 {{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}  1 .text         00000000 0000000000000000 TEXT{{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}  2 .data         00000000 0000000000000000 DATA{{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}  3 .bss          00000000 0000000000000000 BSS{{$}}
+# WHITESPACE-NO-LMA-NEXT: {{^}}  4 .other        00000000 0000000000000000 {{$}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:  .text
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC, SHF_EXECINSTR]
+  - Name:  .data
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC, SHF_WRITE]
+  - Name:  .bss
+    Type:  SHT_NOBITS
+    Flags: [SHF_ALLOC, SHF_WRITE]
+  - Name:  .other
+    Type:  SHT_REL
+
+## The name field automatically expands past the default 13 columns when a
+## section name is longer than that.
+## Note: check with and without --show-lma to make sure resizing happens in
+## both cases.
+
+# RUN: yaml2obj %s --docnum=2 -o %t-name13chars.o
+# RUN: llvm-objdump -h --show-lma %t-name13chars.o \
+# RUN:   | FileCheck %s --check-prefix=NAME-13 --strict-whitespace
+# RUN: llvm-objdump -h %t-name13chars.o \
+# RUN:   | FileCheck %s --check-prefix=NAME-13 --strict-whitespace
+
+# NAME-13:      Sections:
+# NAME-13-NEXT: {{^}}Idx Name          Size
+# NAME-13-NEXT: {{^}}  0               00000000
+# NAME-13-NEXT: {{^}}  1 .foo          00000000
+# NAME-13-NEXT: {{^}}  2 .thirteenchar 00000000
+# NAME-13-NEXT: {{^}}  3 .bar          00000000
+
+## Including a section with a name of length 14 expands the width of the "Name"
+## column.
+# RUN: yaml2obj %s --docnum=3 -o %t-name14chars.o
+# RUN: llvm-objdump -h --show-lma %t-name14chars.o \
+# RUN:   | FileCheck %s --check-prefix=NAME-14 --strict-whitespace
+# RUN: llvm-objdump -h %t-name14chars.o \
+# RUN:   | FileCheck %s --check-prefix=NAME-14 --strict-whitespace
+
+# NAME-14:      Sections:
+# NAME-14-NEXT: {{^}}Idx Name           Size
+# NAME-14-NEXT: {{^}}  0                00000000
+# NAME-14-NEXT: {{^}}  1 .foo           00000000
+# NAME-14-NEXT: {{^}}  2 .fourteenchars 00000000
+# NAME-14-NEXT: {{^}}  3 .bar           00000000
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: .thirteenchar
+    Type: SHT_PROGBITS
+  - Name: .bar
+    Type: SHT_PROGBITS
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: .fourteenchars
+    Type: SHT_PROGBITS
+  - Name: .bar
+    Type: SHT_PROGBITS
+
+## Check that objdump -h only prints the necessary amount of bytes for
+## addresses.
+
+# RUN: yaml2obj %s --docnum=4 -o %t-32bit.o
+# RUN: yaml2obj %s --docnum=5 -o %t-64bit.o
+
+# RUN: llvm-objdump -h --show-lma %t-32bit.o \
+# RUN:   | FileCheck %s --check-prefix=32 --strict-whitespace
+# 32: {{^}}Idx Name          Size     VMA      LMA      Type{{$}}
+# 32: {{^}}  1 .foo          00000000 00000400 00000400 TEXT{{$}}
+
+# RUN: llvm-objdump -h %t-32bit.o \
+# RUN:   | FileCheck %s --check-prefix=32-NO-LMA --strict-whitespace
+# 32-NO-LMA: {{^}}Idx Name          Size     VMA      Type{{$}}
+# 32-NO-LMA: {{^}}  1 .foo          00000000 00000400 TEXT{{$}}
+
+# RUN: llvm-objdump -h --show-lma %t-64bit.o \
+# RUN:   | FileCheck %s --check-prefix=64 --strict-whitespace
+# 64: {{^}}Idx Name          Size     VMA              LMA              Type{{$}}
+# 64: {{^}}  1 .foo          00000000 0000000000000400 0000000000000400 TEXT{{$}}
+
+# RUN: llvm-objdump -h %t-64bit.o \
+# RUN:   | FileCheck %s --check-prefix=64-NO-LMA --strict-whitespace
+# 64-NO-LMA: {{^}}Idx Name          Size     VMA              Type{{$}}
+# 64-NO-LMA: {{^}}  1 .foo          00000000 0000000000000400 TEXT{{$}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_386
+Sections:
+  - Name:    .foo
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_EXECINSTR]
+    Address: 0x400
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .foo
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_EXECINSTR]
+    Address: 0x400

llvm/test/tools/llvm-objdump/wasm.txt

 # RUN: llvm-objdump -h %p/Inputs/trivial.obj.wasm | FileCheck %s
 
 # CHECK:      Sections:
-# CHECK-NEXT: Idx Name          Size    VMA              Type
-# CHECK-NEXT:  0 TYPE          00000011 0000000000000000
-# CHECK-NEXT:  1 IMPORT        0000005d 0000000000000000
-# CHECK-NEXT:  2 FUNCTION      00000003 0000000000000000
-# CHECK-NEXT:  3 CODE          00000024 0000000000000000 TEXT
-# CHECK-NEXT:  4 DATA          0000001c 0000000000000000 DATA
-# CHECK-NEXT:  5 linking       0000006d 0000000000000000
-# CHECK-NEXT:  6 reloc.CODE    0000000f 0000000000000000
+# CHECK-NEXT: Idx Name          Size     VMA      Type
+# CHECK-NEXT:   0 TYPE          00000011 00000000
+# CHECK-NEXT:   1 IMPORT        0000005d 00000000
+# CHECK-NEXT:   2 FUNCTION      00000003 00000000
+# CHECK-NEXT:   3 CODE          00000024 00000000 TEXT
+# CHECK-NEXT:   4 DATA          0000001c 00000000 DATA
+# CHECK-NEXT:   5 linking       0000006d 00000000
+# CHECK-NEXT:   6 reloc.CODE    0000000f 00000000
 
 # RUN: llvm-objdump -p %p/Inputs/trivial.obj.wasm | FileCheck %s -check-prefix CHECK-HEADER
 
 # CHECK-HEADER: Program Header:
 # CHECK-HEADER: Version: 0x1
 
 # RUN: llvm-objdump -s --section=CODE %p/Inputs/trivial.obj.wasm | FileCheck %s -check-prefix CHECK-SECTIONS
 
 # CHECK-SECTIONS: Contents of section CODE:
 # CHECK-SECTIONS: 0000 02170041 80808080 00108080 8080001a  ...A............
 # CHECK-SECTIONS: 0010 10838080 80004100 0b0a0041 00108180  ......A....A....
 # CHECK-SECTIONS: 0020 8080000b                             ....

llvm/test/tools/llvm-objdump/xcoff-section-headers.test

 # RUN: llvm-objdump --section-headers %p/Inputs/xcoff-section-headers.o | \
 # RUN: FileCheck %s
 
 # RUN: llvm-objdump --section-headers %p/Inputs/xcoff-long-sec-names.o | \
 # RUN: FileCheck --check-prefix=LONG %s
 
 # RUN: not llvm-objdump --section-headers 2>&1 \
 # RUN: %p/Inputs/xcoff-section-headers-truncate.o | FileCheck \
 # RUN: --check-prefix=ERROR %s
 
 # ERROR: The end of the file was unexpectedly encountered
 
 # CHECK: xcoff-section-headers.o:      file format aixcoff-rs6000
 # CHECK: Sections:
-# CHECK: Idx Name          Size     VMA          Type
-# CHECK:   1 .text         00000080 0000000000000000 TEXT
-# CHECK:   2 .data         00000024 0000000000000080 DATA
-# CHECK:   3 .bss          00000004 00000000000000a4 BSS
-# CHECK:   4 .tdata        00000008 0000000000000000 DATA
-# CHECK:   5 .tbss         00000004 0000000000000008 BSS
+# CHECK: Idx Name          Size     VMA      Type
+# CHECK:   1 .text         00000080 00000000 TEXT
+# CHECK:   2 .data         00000024 00000080 DATA
+# CHECK:   3 .bss          00000004 000000a4 BSS
+# CHECK:   4 .tdata        00000008 00000000 DATA
+# CHECK:   5 .tbss         00000004 00000008 BSS
 
 # xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 # test.c:
 # int a;
 # int b = 12345;
 # __thread int c;
 # __thread double d = 3.14159;
 #
 # int func(void)  {
 #   return a;
 # }
 
 # LONG: xcoff-long-sec-names.o:      file format aixcoff-rs6000
 # LONG: Sections:
-# LONG: Idx Name          Size     VMA          Type
-# LONG: 1 .dwarnge      00000004 0000000000000000
-# LONG: 2 .dwpbnms      00000004 0000000000000000
-# LONG: 3 .dwpbtyp      00000004 0000000000000000
+# LONG: Idx Name        Size     VMA      Type
+# LONG: 1 .dwarnge      00000004 00000000
+# LONG: 2 .dwpbnms      00000004 00000000
+# LONG: 3 .dwpbtyp      00000004 00000000
 
 # xcoff-long-sec-names.o was generated by assembling the following .s file:
 #  .dwsect 0x30000 # .dwpbnms section
 #  .dwsect 0x40000 # .dwpbtyp section
 #  .dwsect 0x50000 # .dwarnge section

llvm/tools/llvm-objdump/llvm-objdump.cpp

static bool shouldDisplayLMA(const ObjectFile *Obj) {
   if (!Obj->isELF())
     return false;
   for (const SectionRef &S : ToolSectionFilter(*Obj))
     if (S.getAddress() != getELFSectionLMA(S))
       return true;
   return ShowLMA;
 }
 
+static size_t getMaxSectionNameWidth(const ObjectFile *Obj) {
+  // Default column width for names is 13 even if no names are that long.
+  size_t MaxWidth = 13;
+  for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
+    StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());
+    MaxWidth = std::max(MaxWidth, Name.size());
+  }
+  return MaxWidth;
+}
+
 void printSectionHeaders(const ObjectFile *Obj) {
+  size_t NameWidth = getMaxSectionNameWidth(Obj);
+  size_t AddressWidth = 2 * Obj->getBytesInAddress();
   bool HasLMAColumn = shouldDisplayLMA(Obj);
   if (HasLMAColumn)
     outs() << "Sections:\n"
-              "Idx Name          Size     VMA              LMA              "
-              "Type\n";
+              "Idx "
+           << left_justify("Name", NameWidth) << " Size     "
+           << left_justify("VMA", AddressWidth) << " "
+           << left_justify("LMA", AddressWidth) << " Type\n";
   else
     outs() << "Sections:\n"
-              "Idx Name          Size     VMA          Type\n";
+              "Idx "
+           << left_justify("Name", NameWidth) << " Size     "
+           << left_justify("VMA", AddressWidth) << " Type\n";
 
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());
     uint64_t VMA = Section.getAddress();
     if (shouldAdjustVA(Section))
       VMA += AdjustVMA;
 
     uint64_t Size = Section.getSize();
-    bool Text = Section.isText();
-    bool Data = Section.isData();
-    bool BSS = Section.isBSS();
-    std::string Type = (std::string(Text ? "TEXT " : "") +
-                        (Data ? "DATA " : "") + (BSS ? "BSS" : ""));
+
+    std::string Type = Section.isText() ? "TEXT" : "";
+    if (Section.isData())
+      Type += Type.empty() ? "DATA" : " DATA";
+    if (Section.isBSS())
+      Type += Type.empty() ? "BSS" : " BSS";
 
     if (HasLMAColumn)
-      outs() << format("%3d %-13s %08" PRIx64 " %016" PRIx64 " %016" PRIx64
-                       " %s\n",
-                       (unsigned)Section.getIndex(), Name.str().c_str(), Size,
-                       VMA, getELFSectionLMA(Section), Type.c_str());
-    else
-      outs() << format("%3d %-13s %08" PRIx64 " %016" PRIx64 " %s\n",
-                       (unsigned)Section.getIndex(), Name.str().c_str(), Size,
-                       VMA, Type.c_str());
+      outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),
+                       NameWidth, Name.str().c_str(), Size)
+             << format_hex_no_prefix(VMA, AddressWidth) << " "
+             << format_hex_no_prefix(getELFSectionLMA(Section), AddressWidth)
+             << " " << Type << "\n";
+    else
+      outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),
+                       NameWidth, Name.str().c_str(), Size)
+             << format_hex_no_prefix(VMA, AddressWidth) << " " << Type << "\n";
   }
   outs() << "\n";
 }
 
 void printSectionContents(const ObjectFile *Obj) {
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());
     uint64_t BaseAddr = Section.getAddress();