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

[Djordje Todorovic via Phabricator via llvm-commits]

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

@jhenderson Thanks for the review again!



================
Comment at: llvm/include/llvm/Object/ObjectFile.h:278
   virtual bool isBerkeleyData(DataRefImpl Sec) const;
+  virtual bool isDebugSection(StringRef SectionName) const;
   virtual relocation_iterator section_rel_begin(DataRefImpl Sec) const = 0;
----------------
jhenderson wrote:
> Perhaps this interface extension can be pulled into a separate review. You could then modify llvm-objcopy in that same patch to make use of it.
Done with the D76276.

But I have not refactor the llvm-objcopy part, yet. That could be a separate patch.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:15-16
+CHECK-EMPTY:
+CHECK-NEXT: Total Size: 216  (13.78%)
+CHECK-NEXT: Total File Size: 1568
+CHECK-NEXT:----------------------------------------------------
----------------
jhenderson wrote:
> For consistency, it probably makes sense to have these formatted the same as the section names, and have the Section column width take account of this too. I don't feel strongly about this though, so if you prefer it this way, that's fine.
I would keep as is, and we can modify it if starts looking ugly. :)


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:1
+## Check how llvm-dwarfdump calculates section sizes with --show-section-sizes.
+
----------------
jhenderson wrote:
> In another test file possibly, you should test the behaviour where there are no debug sections. Maybe also where all sections are empty? Not sure if that's really important though.
Sure, that makes sense!


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:16
+  // The minimum column width should be the size of "SECTION" plus 2.
+  size_t MinSectionColWidth = SectionNameColPrintName.size() + 2;
+
----------------
jhenderson wrote:
> I don't think you need to pass in `SectionNameColPrintName` for this purpose. Probably simpler would be to factor out a simple local string constant `const StringRef = "Section"`.
> 
> I still don't understand the +2. If all the section names are shorter than "Section", then the column will be two bytes wider than necessary. However, this +2 is not included if one or more of the section names is longer than "Section" + 2. If you want to make the column 2 wider than the names, that's fine (but why?), but you should add the +2 before returning, rather than in the initial value.
>I still don't understand the +2. If all the section names are shorter than "Section", then the column will be two bytes wider than necessary. However, this +2 is not included if one or more of the section names is longer than "Section" + 2. If you want to make the column 2 wider than the names, that's fine (but why?), but you should add the +2 before returning, rather than in the initial value.

OK, I do not have a strong opinion on this, and I will agree on that. Thanks.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:30
+    size_t NumWidth = std::to_string(DebugSec.getValue()).size();
+    MaxNumberWidth = std::max(MaxNumberWidth, NumWidth);
+  }
----------------
jhenderson wrote:
> You need to allow for the percentage part of the size too, I think.
Please take a look into the output now, if it still looks it should be included, I will include the dashes for the percentage part as well.
I think the percentage is only follow-up for the size info, and I feel like not including the dashes somehow emphasize that, but again, I do not feel I have a strong opinion about that, so I am open to change that :)


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:38
+/// Get the maximum section size bytes number width of the found debug sections.
+size_t getMaxSectionSizeNumberWidth(const SectionSizes &Sizes);
+
----------------
jhenderson wrote:
> Similar coment to the above. How about simply `getSizeColumnWidth`?
It looks better, thanks!


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

https://reviews.llvm.org/D74205