[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/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
----------------
As this test is specifically for mach fat archives, perhaps worth renaming the test "fat_archive_section_sizes.test" or possibly "macho_archive_section_sizes.test".


================
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;
----------------
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.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:25
+                             raw_ostream &OS) {
+  size_t MaxSectionWidth = getMaxSectionNameWidth(Sizes);
+
----------------
I'd go with "MaxNameWidth".


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:29
+     << "\n";
+  OS << "  SECTION";
+  for (unsigned i = 0; i < MaxSectionWidth + 2; i++)
----------------
I would suggest that the start of "SECTION" lines up with the start of the section names, to match the sizes column.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:56-60
+  return SectionName.startswith(".debug") ||
+         SectionName.startswith("__debug") ||
+         SectionName.startswith(".zdebug") ||
+         SectionName.startswith("__zdebug") || SectionName == ".gdb_index" ||
+         SectionName == "__gdb_index";
----------------
I think the naming here needs to be format specific unfortunately, as "__debug" is not a debug section for ELF. Perhaps not worth worrying about though...?


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:35
+
+/// Get the maximum section name width of the debug sections found.
+size_t getMaxSectionNameWidth(const SectionSizes &Sizes);
----------------
the found debug sections


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:648
       handleFile(Object, collectStatsForObjectFile, OutputFile.os());
-  else
+  } else if (ShowSectionSizes) {
+    for (auto Object : Objects)
----------------
djtodoro wrote:
> jhenderson wrote:
> > A thought: is there any reason we can't allow --statistics and --show-section-sizes in the same run?
> The option should act as "pretty printing", I think we should not allow mixing the output with `--statistics` JSON output. But, we can include these numbers within `--statistics` output as well.
Okay, sounds reasonable.

Since one of these ifs already uses braces, I recommend all within this bit do for consistency.


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

https://reviews.llvm.org/D74205