[PATCH] D74205: [WIP][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.

@jhenderson	Thanks a lot for your comments! I'll try address them all, and remove the `WIP` from the title.



================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:144
+
+static bool isDebugSection(StringRef SectionName) {
+  if (isDebugInfo(SectionName) || isDebugStr(SectionName) ||
----------------
jhenderson wrote:
> 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.
Thanks!


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:167-168
+
+    if (Section.isBerkeleyText() || Section.isBerkeleyData() ||
+        Section.isBSS() || isDebugSection(SectionName))
+      Sizes.TotalObjectSize += Section.getSize();
----------------
jhenderson wrote:
> 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).
>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.

I saw that `llvm-size` considers as 'total', but I agree this should be revisited. Thanks!

>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).

I see.. Anyhow, better option might be to using the "total object size".


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:174
+
+    if (isDebugInfo(SectionName)) {
+      Sizes.TotalDebugInfo += Section.getSize();
----------------
jhenderson wrote:
> 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.
I put it as `WIP`, and did not have much time to refactor the code, but I will do that and try to remove the `WIP` marker.
Thanks a lot for the suggestion!


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

https://reviews.llvm.org/D74205