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

[Djordje Todorovic via Phabricator via llvm-commits]

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

Thanks!



================
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
----------------
jhenderson wrote:
> 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.
>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).
There are cases, even in llvm-dwarfdump, using the files from Input/, but if you think so, I can delete it.

>The new file should have section names in the ELF style too, to show that they are not included.
I've added a new test case for this.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:6
+CHECK-NEXT:----------------------------------------------------
+CHECK-NEXT:SECTION         SIZE (b)
+CHECK-NEXT:--------------  --------
----------------
jhenderson wrote:
> I might suggest `(bytes)` explicitly, for clarity, but I don't mind, if there's some pre-existing style this is following elsewhere.
I see it is being used as is. Also, I put an explanation in the 'help' for the option.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:9
+CHECK-NEXT:__debug_info          60 (3.83%)
+CHECK-NEXT:__debug_loc            0 (0.00%)
+CHECK-NEXT:__debug_ranges         0 (0.00%)
----------------
aprantl wrote:
> long-term for the human readable output we probably want a function that takes byte value and prints it the way `ls -l -h` formats number, if we don't already have one in LLVM.
I agree.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:80-81
+
+namespace llvm {
+
+void calculateSectionSizes(const ObjectFile &Obj, SectionSizes &Sizes,
----------------
jhenderson wrote:
> You still don't need the `namespace llvm` since you have a `using namespace llvm` at the top of this file.
This is needed to avoid an ambiguous call to the `calculateSectionSizes()`.


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

https://reviews.llvm.org/D74205