[PATCH] D65585: WIP: [llvm-locstats] Add the llvm-locstats tool

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:29
+
+/// This represents the largest category of debug location coverage being
+/// calculated. The first category is 0% location coverage, but the last
----------------
largest category -> number of categories


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:32
+/// category is 100% location coverage.
+static const int largest_cov_category = 12;
+
----------------
constexpr?


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:44
+                      cat(LocStatsCategory));
+static opt<std::string>
+    InputFilename(Positional, desc("<input object file>"),
----------------
`list<std::string>`?


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:116
+  if (auto name = Die.getName(DINameKind::ShortName))
+    LLVM_DEBUG(llvm::dbgs() << "    -var (or formal param): " << name << "\n");
+
----------------
Place if inside LLVM_DEBUG otherwise gcc -Wall warns [-Wunused-variable]


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:120
+
+  auto IsEntryValue = [&] (StringRef D) -> bool {
+    DWARFUnit *U = Die.getDwarfUnit();
----------------
`[&](StringRef D)`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:123
+    DataExtractor Data(D, DICtx.isLittleEndian(), 0);
+    DWARFExpression Expression(Data, U->getVersion(),
+                               U->getAddressByteSize());
----------------
clang-format this line


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:125
+                               U->getAddressByteSize());
+    bool IsEntryVal = llvm::any_of(Expression,
+                                  [](DWARFExpression::Operation &Op) {
----------------
Delete `IsEntryVal`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:138
+    // Handle variables and function arguments location.
+    auto FormValue = Die.find(dwarf::DW_AT_location);
+    if (FormValue.hasValue()) {
----------------
`if (auto FormValue = ...)`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:145
+        if (auto List = DebugLoc->getLocationListAtOffset(*DebugLocOffset)) {
+          for (auto Entry : List->Entries) {
+            if (IgnoreEntryValues &&
----------------
`auto` -> `DWARFDebugLoc::Entry`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:147
+            if (IgnoreEntryValues &&
+                IsEntryValue({Entry.Loc.data(), Entry.Loc.size()}))
+              continue;
----------------
IsEntryValue(Entry.Loc)

(I just changed the type of Loc to SmallString.)


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:183
+  LocStatistics[PercentageKey]++;
+  CumulNumOfVars++;
+}
----------------
`++CumulNumOfVars;`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:228
+    uint64_t BytesInThisScope = 0;
+    for (auto Range : Ranges)
+      BytesInThisScope += Range.HighPC - Range.LowPC;
----------------
`auto` -> `DWARFAddressRange`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:243
+  DWARFDie Child = Die.getFirstChild();
+  while (Child) {
+    collectStatsRecursive(Child, ScopeLowPC, BytesInScope, LocStatistics,
----------------
`for (DWARFDie Child : Die)`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:264
+  OS << "    0"
+     << "         " << format_decimal(LocStatistics[0], 8) << "        "
+     << format_decimal((int)(LocStatistics[0] / (double)CumulNumOfVars * 100),
----------------
`formatv("   {0,8}  {1,8}", ..., ...)` might be clearer


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:294
+  // Map percentage->occurrences.
+  std::map<int, unsigned long> LocStatistics;
+  for (int i = 0; i < largest_cov_category; ++i)
----------------
`std::vector<int> LocStatistics(largest_cov_category, 0);`


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:317
+
+static void handleBuffer(StringRef Filename, MemoryBufferRef Buffer,
+                         HandlerFn HandleObj, raw_ostream &OS) {
----------------
This function is also a candidate for inlining.


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:328
+
+static void handleFile(StringRef Filename, HandlerFn HandleObj,
+                       raw_ostream &OS) {
----------------
Can this function be inlined? (only called once)


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:331
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BuffOrErr =
+  MemoryBuffer::getFileOrSTDIN(Filename);
+  error(Filename, BuffOrErr.getError());
----------------
Indent this line


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:334
+  std::unique_ptr<MemoryBuffer> Buffer = std::move(BuffOrErr.get());
+  handleBuffer(Filename, *Buffer, HandleObj, OS);
+}
----------------
`*Buffer` -> `**BuffOrErr` and delete `Buffer`.


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:358
+
+  if (OnlyFormalParameters && OnlyVariables) {
+    WithColor::error() << "incompatible arguments: specifying both "
----------------
Ideally these switches can be ORed together.


================
Comment at: llvm/tools/llvm-locstats/llvm-locstats.cpp:372
+  handleFile(InputFilename, collectLocstats, OutputFile.os());
+ 
+  return EXIT_SUCCESS;
----------------
Trailing space.


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

https://reviews.llvm.org/D65585