[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.
+
----------------
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?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:4
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-dwarfdump --show-section-sizes %t | FileCheck %s --match-full-lines
+
----------------
I'd still like to add --strict-whitespace to this test. This ensures that the formatting is as expected, since without it, all whitespace is treated as a single space and therefore things like indentation may mess up.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:16
+# CHECK-EMPTY:
+# CHECK-NEXT:  all debug sections:    58  (8.15%)
+# CHECK-NEXT: ----------------------------------------------------
----------------
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.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:30
+    Type:         SHT_PROGBITS
+    Content:      00636C616E672076657273696F6E20332E
+  - Name:         .debug_line
----------------
You can use the Size: parameter instead of Content:, here and below, to specify a section size. It'll get filled with zeroes, but it's not important to have valid DWARF for this.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:33
+
+static void reportWarning(Error Err, Twine &Filename) {
+  assert(Err);
----------------
WithColor.h has recently gained `defaultWarningHandler` which allows printing of errors nicely, without the extra effort here.

Also, there is a `createFileError` method that takes an Error. Consequently, you can write this function simply as:
```
WithColor::defaultWarningHandler(createFileError(Filename, std::move(Err));
```

You could even consider doing away with this function then, by doing this inline, but it's up to you on that one. I'm happy either way.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:75
+static void calculateSizes(const ObjectFile &Obj, SectionSizes &Sizes,
+                           Twine &Filename) {
+  // Get total size.
----------------
Here and throughout your code, use `const Twine &`. From the Twine.h comments:

"Twines should only be used accepted as const references in arguments,"


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:97
+
+bool collectSecsSizesForObjectFile(ObjectFile &Obj, Twine Filename,
+                                   raw_ostream &OS) {
----------------
`collectSecsSizesForObjectFile` -> `collectObjectSectionSizes`


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:105
+  OS << "----------------------------------------------------\n";
+  OS << Filename.str() << ": file format " << Obj.getFileFormatName() << "\n";
+
----------------
Is the file format really important? I personally don't think it gives us anything.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:287
                                      raw_ostream &)>;
+using HandlerSecSizesFn =
+    std::function<bool(ObjectFile &, Twine, raw_ostream &)>;
----------------
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.


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

https://reviews.llvm.org/D74205