[PATCH] D74205: [llvm-dwarfdump] Add the --show-sections-sizes option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:1
+# Check how llvm-dwarfdump calculates section sizes with --show-section-sizes.
+
----------------
djtodoro wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > I like to encourage people to use '##' for comments in tests these days, as they stand out better from the main lit and FileCheck directives.
> > > 
> > > Aside from that, I'd suggest some improvements to this test:
> > > 
> > > 1) Check the whole output, and also use FileCheck's --match-full-lines and --strict-whitespace to show that the formatting works.
> > > 2) Have multiple of one of the sections, to show that the size is totalled up. It is possible to have multiple such sections (e.g. due to multiple .debug_types sections).
> > > 3) Use different sizes for the sections, so that it clearly shows that the size is calculated correctly.
> > > 4) Add a section that starts with ".debug" but doesn't have a "standard" dwarf name (e.g. .debugfoo or .debug_bar), and that it is still printed.
> > I don't think you've addressed point 2?
> I am getting the YAML error when trying to add duplicated section here:
> 
> `repeated section/fill name: ... at YAML section/fill number ...`
There's a special syntax for creating unique sections with the same name. See case 1 in https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/ELF/duplicate-section-names.yaml.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:6
+
+# CHECK:file: {{.*}}
+# CHECK-NEXT:----------------------------------------------------
----------------
Tip: Add spaces between the '#' and 'CHECK' so that the colons line up:
```
#      CHECK:file: {{.*}}
# CHECK-NEXT:----------------------------------------------------
```


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:10
+# CHECK-NEXT:------------        -----------------------
+# CHECK-NEXT: .debug_foo             100        (12.38%)
+# CHECK-NEXT: .debug_info             17        (2.10%)
----------------
I think the section name column needs to be dynamically sized. The length of a section name isn't really limited, and indeed, there are some quite long DWARF section names out there already (e.g. .debug_cu_index or .debug_info.dwo), which might cause the formatting here to go to pot. Please add a section with a long name (e.g. change .debug_foo to .debug_arbitrary_long_name) to illustrate.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:16
+# CHECK-EMPTY:
+# CHECK-NEXT:  all debug sections:    58  (8.15%)
+# CHECK-NEXT: ----------------------------------------------------
----------------
jhenderson wrote:
> I think you can simply say "total debug:" here.
> I'd also like the Size values and percentage values to line up if possible, to look neat.
> 
> Finally, it would make sense in this test for at least one of the percentages to be >= 10% to show the behaviour for wider percentage values.
Actually, on second thoughts "Total Size" (without the colon) is probably better. I think it reads cleaner.

I'd also space it the same as the section columns above, so that it all lines up.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:105
+  OS << "----------------------------------------------------\n";
+  OS << Filename.str() << ": file format " << Obj.getFileFormatName() << "\n";
+
----------------
djtodoro wrote:
> jhenderson wrote:
> > Is the file format really important? I personally don't think it gives us anything.
> Hm... OK. I see the llvm-dwarfdump prints the file format when outputs the `--stats`, but we can avoid that here.
If a use-case arises for it later, we can add it then.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:89
+bool collectObjectSectionSizes(ObjectFile &Obj, DWARFContext &DICtx,
+                               Twine Filename, raw_ostream &OS) {
+  // Unused, but we want to keep using the llvm-dwarfdump function handler
----------------
`const Twine &`
`DICtx` -> `/*DICtx*/` and then you don't need the comment or void cast below.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74205/new/

https://reviews.llvm.org/D74205