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

Mon Mar 16 03:53:15 PDT 2020

jhenderson added inline comments.

================
Comment at: llvm/include/llvm/Object/ObjectFile.h:126

+  /// Whether this section is debug section.
+  bool isDebugSection(StringRef SectionName) const;
----------------
is a debug

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

================
Comment at: llvm/lib/Object/ObjectFile.cpp:94

+bool ObjectFile::isDebugSection(StringRef SectionName) const {
+  return SectionName.startswith(".debug") ||
----------------
I think this function needs to be implemented separately for ELF/Mach-O etc. For ELF objects, we don't want __debug counting as a debug section, whilst for Mach-O we don't want .debug. And for COFF etc, we don't want either probably!

It should be easy to make this a virtual function overridden by the object file types.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:7
+CHECK-NEXT:SECTION         SIZE
+CHECK-NEXT:--------------  --
+CHECK-NEXT:__debug_info    60 (3.83%)
----------------
I'd expect the dashes to go to the closing parenthesis for the percentage.

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

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:1
+## Check how llvm-dwarfdump calculates section sizes with --show-section-sizes.
+
----------------
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.

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

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:27
+size_t getMaxSectionSizeNumberWidth(const SectionSizes &Sizes) {
+  size_t MaxNumberWidth = 1;
+  for (const auto &DebugSec : Sizes.DebugSectionSizes) {
----------------
Let's make this consistent with my suggestions above, and use the column title as the minimum width.

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:30
+    size_t NumWidth = std::to_string(DebugSec.getValue()).size();
+    MaxNumberWidth = std::max(MaxNumberWidth, NumWidth);
+  }
----------------
You need to allow for the percentage part of the size too, I think.

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:39
+  StringRef SectionNameColPrintName = "SECTION";
+  StringRef SecttonSizePrintName = "SIZE";
+
----------------
Typo in variable name.

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:112-113
+
+  // TODO: If it's an archive, print the cumulative summary of all files from
+  // the archive.
+
----------------
Clarify what "it's" is here (i.e. use "If the input file is an archive")

Also, I'd move the TODO below the function call to print.

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:17-18
+
+using namespace llvm;
+using namespace object;
+
----------------
I don't think it's good form to put `using namespace` in a header file. Wrap the whole thing inside `namespace llvm` etc.

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:20
+
+using SectionSizeMap = llvm::StringMap<uint64_t>;
+
----------------
No need for the `llvm::`

================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:34
+/// Get the maximum section name width of the found debug sections.
+size_t getMaxSectionNameWidth(const SectionSizes &Sizes,
+                              StringRef SectionNameColPrintName);
----------------
Maybe worth calling this `getNameColumnWidth` and updating the comment accordingly.

================
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);
+
----------------
Similar coment to the above. How about simply `getSizeColumnWidth`?

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

https://reviews.llvm.org/D74205