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

@aprantl @jhenderson Thanks a lot for the reviews! Now we have tests for ELF(section_sizes.test), COFF(coff-x86_64.yaml) and MACH-O(macho_archive_section_sizes.test).



================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:6
+CHECK-NEXT:----------------------------------------------------
+CHECK-NEXT:SECTION         SIZE
+CHECK-NEXT:--------------  ----
----------------
aprantl wrote:
> What's the unit of SIZE? Should we print that?
bytes, and we can hard code them now, but when we add support for different units, that could be dynamically printed.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:24
+CHECK-NEXT:__debug_info    56   (4.00%)
+CHECK-NEXT:__debug_loc     0    (0.00%)
+CHECK-NEXT:__debug_ranges  0    (0.00%)
----------------
aprantl wrote:
> Should we right-align all the numbers?
> 
> ```
> 56
>  0
> ```
Good idea, thanks!


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_no_debug_sections.test:6
+# CHECK-NEXT:----------------------------------------------------
+# CHECK-NEXT:No debug sections found.
+# CHECK-NEXT:----------------------------------------------------
----------------
jhenderson wrote:
> Rather than having a special message here, I'd probably just print the Total Size and Total File Size lines as before (which will be 0 and the object file size).
Sounds good, thanks!


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:31
+size_t getSizeColumnWidth(const SectionSizes &Sizes,
+                          const StringRef SecttonSizeTitle) {
+  // The minimum column width should be the size of "SIZE".
----------------
jhenderson wrote:
> Typo: `SectionSizeTitle`
Oh, I uploaded a wrong diff :(


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

https://reviews.llvm.org/D74205