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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 3 inline comments as done.
djtodoro 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.
+
----------------
jhenderson wrote:
> 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.
Thanks!


================
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%)
----------------
jhenderson wrote:
> 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.
It makes sense.


================
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
----------------
jhenderson wrote:
> `const Twine &`
> `DICtx` -> `/*DICtx*/` and then you don't need the comment or void cast below.
> `const Twine &`
It is addressed along with the D75727.


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

https://reviews.llvm.org/D74205