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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 10 02:20:03 PST 2020


jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-dwarfdump.rst:113
 
+.. option:: --show-section-sizes
+
----------------
Options are documented in alphabetical order, so this should be above --statistics.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/show_section_sizes.s:1
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj | llvm-dwarfdump --show-section-sizes - | FileCheck %s
+#
----------------
As this test doesn't do any analysis it would make more sense to use yaml2obj to create sections of an appropriate size instead of llvm-mc on a bunch of hard-to-read assembly. You can see examples in something like "llvm\test\tools\llvm-readobj\ELF\sections.test".


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/show_section_sizes.s:2
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj | llvm-dwarfdump --show-section-sizes - | FileCheck %s
+#
+# CHECK: .debug_info 132
----------------
No need for the '#' character on empty lines (here and at the bottom of the checks).


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:144
+
+static bool isDebugSection(StringRef SectionName) {
+  if (isDebugInfo(SectionName) || isDebugStr(SectionName) ||
----------------
I think this can be made much more general, robust, and future-proof by doing what llvm-objcopy does and simply checking that name starts with ".debug" and other related strings (which you may or may not wish to include). See `isDebugSection` in ELFObjcopy.cpp.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:161
+    } else {
+      consumeError(NameOrErr.takeError());
+      return false;
----------------
I'd report a warning or error here, rather than just throwing away the error.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:167-168
+
+    if (Section.isBerkeleyText() || Section.isBerkeleyData() ||
+        Section.isBSS() || isDebugSection(SectionName))
+      Sizes.TotalObjectSize += Section.getSize();
----------------
Why are you limiting this totalling up to a limited subset of sections? I'd expect this code to either a) return the total size of all sections, or the total object size. Note that not all sections are one of the four categories your code mentions, nor does this include other file data like an ELF header or the section header table itself.

Also, do you really want to include BSS size in this total? That takes up no disk footprint usually, only memory (which debug sections do not take up).


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:174
+
+    if (isDebugInfo(SectionName)) {
+      Sizes.TotalDebugInfo += Section.getSize();
----------------
Rather than this big long block, how about a simpler approach as shown by the following pseudo-code:

```
// std::map rather than std::unordered_map, so that the names appear alphabetically
std::map<StringRef, uin64_t> DebugSectionSizes;
for(Section S : Sections) {
  StringRef Name = S.getName();
  if (!isDebugSection(Name))
    continue;
  StringRef Key;
  if (Name.startswith(".debug"))
    Key = Name.substr(6);
  // other cases for other possible prefixes, e.g. .zdebug
  DebugSectionSizes[Key] += S.size();
}
uint64_t TotalDebugSize = 0;
// Print some header data
for (auto KV : DebugSectionSizes) {
  StringRef Name = KV.first;
  StringRef Size = KV.second;
  TotalDebugSize += Size;
  // Print .debug_ + Name, Size
}
// Print total sizes
```

This will allow it to be simpler, and future-proof against new debug sections. The only possible downside would be that it would only print the sizes of sections that exist. If this is an issue, pre-populate the map with appropriate sections, i.e. something like:

```
std::unordered_map<StringRef, uin64_t> DebugSectionSizes = {
  {"_info", 0}, {"_types", 0}, // etc
};
```

The problem with that idea is that it assumes you know all debug section names that both currently exist and could exist in the future. I don't think this is viable, so I think populating the table dynamically would be better.


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

https://reviews.llvm.org/D74205





More information about the llvm-commits mailing list