[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/section_sizes.test:1
+# Check how llvm-dwarfdump calculates section sizes with --show-section-sizes.
+
----------------
I like to encourage people to use '##' for comments in tests these days, as they stand out better from the main lit and FileCheck directives.

Aside from that, I'd suggest some improvements to this test:

1) Check the whole output, and also use FileCheck's --match-full-lines and --strict-whitespace to show that the formatting works.
2) Have multiple of one of the sections, to show that the size is totalled up. It is possible to have multiple such sections (e.g. due to multiple .debug_types sections).
3) Use different sizes for the sections, so that it clearly shows that the size is calculated correctly.
4) Add a section that starts with ".debug" but doesn't have a "standard" dwarf name (e.g. .debugfoo or .debug_bar), and that it is still printed.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test:20
+    Type:         SHT_PROGBITS
+    AddressAlign: 0x20
+    Content:      02
----------------
No need for the AddressAlign parameter here and below.


================
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
+#
----------------
djtodoro wrote:
> jhenderson wrote:
> > 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".
> I'll add such test as well.
I think you can get rid of this test entirely, in favour of the YAML-based test. There's no need for duplicate coverage.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:24
+struct SectionSizes {
+  /// Map the .debug section and total number of bytes of the debug section.
+  SectionSizeMap DebugSectionSizes;
----------------
Perhaps: "Map of .debug section names and their sizes across all such-named sections."


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:32
+
+static void prettyPrintTheSecsSizes(const ObjectFile &Obj,
+                                    const SectionSizes &Sizes,
----------------
I'd replace "TheSecs" with "Section" in the function name.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:52
+     << "\n";
+  OS << " TOTAL FILE SIZE: " << Sizes.TotalObjectSize << "\n";
+  OS << "----------------------------------------------------"
----------------
As mentioned before, the total size of all sections is not the same as the total file size. I'd recommend just doing `Obj.getData().size()` here.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:68
+    } else {
+      WithColor::error() << errorToErrorCode(NameOrErr.takeError()).message()
+                         << "\n";
----------------
`errorToErrorCode` is not a good way of reporting an llvm::Error as it loses a lot of context that might otherwise be reported. Also, perhaps this should only be a warning and then skipping that section, rather than an error which terminates the loop. After all, it might be possible to find the names of other sections. If you did that, this function could return void.

I'd recommend you pull this out into a function, and probably follow what `reportWarning` does in llvm-readobj.cpp.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:101-105
+  uint64_t TotalCUs = 0;
+  for (const auto &CU : static_cast<DWARFContext *>(&DICtx)->compile_units()) {
+    (void)CU;
+    ++TotalCUs;
+  }
----------------
I'm not sure this loop gives us anything. The number of CUs seems irrelevant to the task at hand.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:650
+    for (auto Object : Objects)
+      handleFile(Object, collectSecsSizesForObjectFile, OutputFile.os());
+  } else {
----------------
It looks like you handle each file individually, which is fine, but it probably makes sense to include the file name somewhere in your output too, to clearly distinguish them. Also, as you can have multiple inputs, you should have a test for that (possibly it can be part of the existing test).


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

https://reviews.llvm.org/D74205