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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Code all looks fine now, just a few minor test suggestions.

What happened to "macho_archive_section_sizes.test"? I thought that was asked for for testing fat archives? On that note, it might be worth a test for llvm-ar produced archives too.



================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test:26
+    Characteristics: []
+    SectionData:     70726F647563657220737472696E6700746573742E63002F706174682F746F2F73726300
+  - Name:            .debug_abbrev
----------------
jhenderson wrote:
> 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.
Perhaps worth having one of these with a non-zero size?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test:36
+    SectionData:     00
+## 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.
+  - Name:            __debug_foo
----------------
Split this over two lines. Also, there was a typo in my original suggestion: "show that it such" -> "show that such"


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_elf.test:49
+    Size: 9
+## 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.
+  - Name: __debug_bar
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_macho.test:39
+    Sections:
+## This is a debug section following the ELF naming style, and is used to show that it such sections are not included in the report.
+      - sectname:        .debug_line
----------------
Same comment as elsewhere.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:80-81
+
+namespace llvm {
+
+void calculateSectionSizes(const ObjectFile &Obj, SectionSizes &Sizes,
----------------
djtodoro wrote:
> jhenderson wrote:
> > 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...
> Without even `llvm::` here I get:
> 
>   SectionSizes.cpp:109:3: error: call to 'calculateSectionSizes' is ambiguous
> 
> maybe I am oversighting something very obvious here?
Never mind, I'm just being a bit dumb, I think.


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

https://reviews.llvm.org/D74205