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

[Djordje Todorovic via Phabricator via llvm-commits]

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

I tested this output on bigger files, i.e. `gdb`:

  llvm-dwarfdump --show-section-sizes gdb
  ----------------------------------------------------
  file: gdb
  ----------------------------------------------------
  SECTION         SIZE
  --------------  --------
  .debug_info     74863947 (63.71%)
  .debug_loc      15324074 (13.04%)
  .debug_ranges   2709232  (2.31%)
  .debug_aranges  66432    (0.06%)
  .debug_abbrev   2103393  (1.79%)
  .debug_line     4517257  (3.84%)
  .debug_str      8194356  (6.97%)
  
   Total Size: 107778691  (91.72%)
   Total File Size: 117510032
  ----------------------------------------------------



================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:14
+size_t getMaxSectionNameWidth(const SectionSizes &Sizes) {
+  // The minimum column length should be the size of "SECTION" plus 2.
+  size_t MaxWidth = 9;
----------------
jhenderson wrote:
> aprantl wrote:
> > compute this as a constexpr?
> length -> width
> 
> Your comment suggests that the size should include the "+ 2", and the initial MaxWidth does so, but then you don't include it in the loop, and the rest of the code adds the 2 on, so I think there's an inconsistency here.
I have change the function a bit, so I think we do not need the "constexpr" anymore.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:14
+size_t getMaxSectionNameWidth(const SectionSizes &Sizes) {
+  // The minimum column length should be the size of "SECTION" plus 2.
+  size_t MaxWidth = 9;
----------------
djtodoro wrote:
> jhenderson wrote:
> > aprantl wrote:
> > > compute this as a constexpr?
> > length -> width
> > 
> > Your comment suggests that the size should include the "+ 2", and the initial MaxWidth does so, but then you don't include it in the loop, and the rest of the code adds the 2 on, so I think there's an inconsistency here.
> I have change the function a bit, so I think we do not need the "constexpr" anymore.
>Your comment suggests that the size should include the "+ 2", and the initial MaxWidth does so, but then you don't include it in the loop, and the rest of the code adds the 2 on, so I think there's an inconsistency here.
Sorry, I am not sure I follow :) Maybe my comment is not clear enough? The "+2" refers only for the minimum width.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:55
+
+bool isDebugSection(StringRef SectionName) {
+  return SectionName.startswith(".debug") ||
----------------
aprantl wrote:
> Shouldn't this be in libObject instead?
Since the ELF does not consider the '__debug*' as debug sections, I would use this locally.


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

https://reviews.llvm.org/D74205