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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 4 inline comments as done.
djtodoro added a comment.

In D74205#1909868 <https://reviews.llvm.org/D74205#1909868>, @aprantl wrote:

> Sorry for taking so long, I have three more questions:
>
> What does the output look like for fat Mach-O binaries? There should be some in the dsymutil test inputs, if you have trouble creating one.
>  What does the output look like for a static `.a` archive?


I have added the tests for this.

> I'm wondering if we should include the *data* collected here in the JSON output of --statistics if we don't already.

We do not include, but I can split the methods from `SectionSizes.cpp` into a header file, and allow the `Statistics` part to using this as well. I think this fields are useful for that. WDYT?



================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:34
+static size_t getMaxSectionNameWidth(const SectionSizes &Sizes) {
+  size_t MaxWidth = 13;
+  for (const auto &DebugSec : Sizes.DebugSectionSizes) {
----------------
jhenderson wrote:
> Why `13` here? I'd think it would only need to have a minimum of the size of "SECTION" + 2 or so.
Sure. The magic value was used within some other LLVM tool when printing the section name, that is why I put it like this.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:79
+    StringRef SectionName;
+    if (Expected<StringRef> NameOrErr = Section.getName()) {
+      SectionName = *NameOrErr;
----------------
SouraVX wrote:
> nit: Since these are one liners. Can remove these braces from both if-else block?
Sure.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:420
 
+bool collectObjectSectionSizes(ObjectFile &Obj, DWARFContext & /*DICtx*/,
+                               Twine Filename, raw_ostream &OS);
----------------
jhenderson wrote:
> I just want to be clear: is this how clang-format formats this comment?
Yes.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:648
       handleFile(Object, collectStatsForObjectFile, OutputFile.os());
-  else
+  } else if (ShowSectionSizes) {
+    for (auto Object : Objects)
----------------
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.


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

https://reviews.llvm.org/D74205