[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/macho_archive_section_sizes.test:1
+RUN: llvm-dwarfdump --show-section-sizes %S/../../dsymutil/Inputs/libfat-test.a \
+RUN:   | FileCheck %s --match-full-lines --strict-whitespace
----------------
You should avoid using a precanned binary if possible, especially one from a completely different tool's testing. yaml2obj has Mach-O support, so I'd recommend using that (although I don't know how to create fat archives).

The new file should have section names in the ELF style too, to show that they are not included.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:6
+CHECK-NEXT:----------------------------------------------------
+CHECK-NEXT:SECTION         SIZE (b)
+CHECK-NEXT:--------------  --------
----------------
I might suggest `(bytes)` explicitly, for clarity, but I don't mind, if there's some pre-existing style this is following elsewhere.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:46
+    Size: 100
+  - Name: .debug_info.dwo
+    Type: SHT_PROGBITS
----------------
You might want to add a section with a Mach-O naming style to show that it is NOT included.


================
Comment at: llvm/test/tools/llvm-dwarfdump/coff-x86_64.yaml:2
 # RUN: yaml2obj %s | llvm-dwarfdump - | FileCheck %s
+# RUN: yaml2obj %s | llvm-dwarfdump - --show-section-sizes \
+# RUN:   | FileCheck %s --check-prefix=SECTION-SIZES --match-full-lines --strict-whitespace
----------------
Rather than folding it in here, I'd keep this test in a separate test file, or even just add it to section_sizes.test (you might need to copy the YAML across too of course).

Same comment then goes as the other tests re. section names.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:29
+                                 const StringRef SectionSizeTitle) {
+  // The minimum column width should be the size of "SIZE (b)".
+  size_t Width = SectionSizeTitle.size();
----------------
To avoid rotting once support for other units is added, it might make more sense to say "the size of the column title".


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:80-81
+
+namespace llvm {
+
+void calculateSectionSizes(const ObjectFile &Obj, SectionSizes &Sizes,
----------------
You still don't need the `namespace llvm` since you have a `using namespace llvm` at the top of this file.


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

https://reviews.llvm.org/D74205