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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 4 inline comments as done.
djtodoro added a comment.

@jhenderson Thanks for the review!



================
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:
> 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 ...`


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:75
+static void calculateSizes(const ObjectFile &Obj, SectionSizes &Sizes,
+                           Twine &Filename) {
+  // Get total size.
----------------
jhenderson wrote:
> Here and throughout your code, use `const Twine &`. From the Twine.h comments:
> 
> "Twines should only be used accepted as const references in arguments,"
Addressed with D75727, since we have more functions using it.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:105
+  OS << "----------------------------------------------------\n";
+  OS << Filename.str() << ": file format " << Obj.getFileFormatName() << "\n";
+
----------------
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.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:287
                                      raw_ostream &)>;
+using HandlerSecSizesFn =
+    std::function<bool(ObjectFile &, Twine, raw_ostream &)>;
----------------
jhenderson wrote:
> This seems unnecessary. The only difference is removing the DWARFContext argument, right? I'd just pass in the DWARFContext and not use it to keep the interface identical.
Agree, it seems reasonable.


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

https://reviews.llvm.org/D74205