[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
----------------
djtodoro wrote:
> 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.
Using assembly etc from the Inputs folder local to the current tool is fine, but where possible, we try to avoid pre-canned binaries these days, as they are opaque and hard to maintain. Referencing anotehr tool's Inputs folder is particularly bad as it creates an unexpected dependency between the two sets of tests.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test:1
+# RUN: yaml2obj %s | llvm-dwarfdump - --show-section-sizes \
+# RUN:   | FileCheck %s --match-full-lines --strict-whitespace
----------------
Add a comment like the other test.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test:26
+    Characteristics: []
+    SectionData:     70726F647563657220737472696E6700746573742E63002F706174682F746F2F73726300
+  - Name:            .debug_abbrev
----------------
If I'm not mistaken, these don't need to be valid data. Does COFF yaml2obj provide the "Size:" tag as an option for setting the section size? If not, I'd just use zeroes to write the data. Same goes below.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test:34
+    SectionData:     170000000400000000000401000000000C00000000000000000000
+## This is the debug section following the Mach-O naming style, so this is not included in the report.
+  - Name:            __debug_foo
----------------
"This is the debug section following the Mach-O naming style, so this is not included in the report." -> "This is a debug section following the Mach-O naming style, and is used to show that it such sections are not included in the report."


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_elf.test:1
+## Check how llvm-dwarfdump calculates section sizes with --show-section-sizes.
+
----------------
Add "for ELF objects" to the end here.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_elf.test:49
+    Size: 9
+## This is the debug section following the Mach-O naming style, so this is not included in the report.
+  - Name: __debug_bar
----------------
Same comment as above.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_mach_o.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-dwarfdump --show-section-sizes %t | FileCheck %s --match-full-lines --strict-whitespace
----------------
Rename this test to section_sizes_macho.test for consistency with typical naming styles elsewhere.

Please add a top-level comment too.

Any particular reason you're creating an intermediate object here and not in the COFF case?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_mach_o.test:37
+    Sections:
+## This is the debug section following the ELF naming style, so this is not included in the report.
+      - sectname:        .debug_line
----------------
Same basic comment as in other tests.


================
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();
----------------
jhenderson wrote:
> To avoid rotting once support for other units is added, it might make more sense to say "the size of the column title".
Sorry, I guess this wasn't quite clear enough. I meant to replace the "size of "SIZE (b)"" with "size of the column title" here. So the full sentence is "The minimum column width should be 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,
----------------
djtodoro wrote:
> 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()`.
What ambiguous call? There is only one function called `calculateSectionSizes()` defined here...


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

https://reviews.llvm.org/D74205