[PATCH] D55091: Add --analyze option to llvm-dwarfdump

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Support/StringExtras.cpp:105-108
+    while (FSize >= 1024.0) {
+      FSize /= 1024.0;
+      I++;
+    }
----------------
Seems like there could be potential for a floating point related error here where something that is an exact power of 1024 (e.g. 1.000K, 1.000M, 1.000G, 1.000T, etc) or very close to an exact multiple gets reported as being of the wrong unit (e.g. Terabytes instead of Gigabytes) due to the conditional.

What about something like:

```
unsigned I = Log2_64(USize) / 10;
uint64_t PowerOf1024 = 1ULL << (I - 1);
uint64_t NextPowerOf1024 = PowerOf1024 << 1;
float Percentage = 1.0f + float(USize - PowerOf1024) / float(NextPowerOf1024 - PowerOf1024);
OS << format("%.2f", Percentage) << Units[I];
```

I'm not sure if I got the math right.  Also not sure if it's a problem in practice.


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:22-25
+static bool isDwarfSectionName(StringRef Name) {
+  return Name.startswith(".debug") || Name.startswith("__debug") ||
+         Name.startswith(".apple") || Name.startswith("__apple");
+}
----------------
If we decide to put the section dumping code in `llvm-objdump` or `llvm-readobj`, it would be nice if it were general enough that it could be used for non-dwarf sections.  What if we wanted to figure out the size of all `.text` sections for example.  


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:81
+
+bool analyzeObjectFile(ObjectFile &Obj, DWARFContext &DICtx, Twine Filename,
+                       raw_ostream &OS) {
----------------
Can you mark this function static?


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:81
+
+bool analyzeObjectFile(ObjectFile &Obj, DWARFContext &DICtx, Twine Filename,
+                       raw_ostream &OS) {
----------------
zturner wrote:
> Can you mark this function static?
`Twine` should always be passed by `const&`, it's actually an error to pass by value.


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:102
+    Sect.getName(Name);
+    const auto SectSize = Sect.getSize();
+    if (isDwarfSectionName(Name)) {
----------------
Please use explicit type spelling here instead of `auto`.


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:116
+      Sect.getName(Name);
+      const auto SectSize = Sect.getSize();
+      json::Object SectObj;
----------------
explicit type spelling.


================
Comment at: tools/llvm-dwarfdump/Analyze.cpp:131
+      Sect.getName(Name);
+      const auto SectSize = Sect.getSize();
+      OS << ReadableFileSize(SectSize, TotalDwarfSize, 7) << ' '
----------------
explicit type spelling.


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

https://reviews.llvm.org/D55091