[PATCH] D120860: [memprof] Filter out callstack frames which cannot be symbolized.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:300
     for (const uint64_t VAddr : Entry.getSecond()) {
-      // Check if we have already symbolized and cached the result.
-      if (SymbolizedFrame.count(VAddr) > 0)
+      // Check if we have already symbolized and cached the result or we don't
+      // want to attempt symbolization since we know this address is bad. In
----------------
Nit: parses slightly better if you say "or if we don't..."


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:315
+      if (DI.getFrame(0).FunctionName == DILineInfo::BadString ||
+          StringRef(DI.getFrame(0).FileName).contains("memprof_malloc_")) {
+        AllVAddrsToDiscard.insert(VAddr);
----------------
I think operator new should go through memprof_new_delete.cpp? Does FileName contain any of the path, or do we have it available anywhere? I.e. could we match on "/memprof/memprof_"?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:336
+    auto &CallStack = Entry.getSecond();
+    CallStack.erase(std::remove_if(CallStack.begin(), CallStack.end(),
+                                   [&AllVAddrsToDiscard](const uint64_t A) {
----------------
Rather than scan CallStack again, can you keep a flag initialized to false above the prior loop, set to true if any entries are symbolized, and check that here?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:37
+We expect 2 MIB entries, 1 each for the malloc calls in the program. Any
+additional allocations which do not originae from the main binary are pruned.
 
----------------
s/originae/originate/


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:45
 CHECK-NEXT:     NumSegments: 9
 CHECK-NEXT:     NumMibInfo: 3
 CHECK-NEXT:     NumStackOffsets: 3
----------------
Should this now be 2? Ditto for NumStackOffsets below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120860