[PATCH] D111902: [llvm-profgen] Warn and filter out invalid range

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:36
+    cl::desc(
+        "Skip the check of fitering out invalid range and warning message."));
 
----------------
hoy wrote:
> nit: name it  `filter-invalid-lbr-ranges" and flip the default value?
> 
I'm curious in what scenario do we not want to filter out invalid ranges? I feels to me that we don't need a switch here? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1044
+            << format("%8" PRIx64, RangeEnd)
+            << "]: Invalid range or disassembling error in profiled binary.\n";
+        RangeToBeRemoved.push_back(I);
----------------
The message is a bit unclear. Does this indicate we have a range for functions outside of the binary? We would have caught diassembling error while diassembling?

perhaps this `Range does not belong to any functions`?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1049
+                             << format("%8" PRIx64, RangeEnd)
+                             << "]: Range is across different functions.\n";
+        RangeToBeRemoved.push_back(I);
----------------
nit, better message: `Fall through range should not cross function boundaries`


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1051
+        RangeToBeRemoved.push_back(I);
+      }
+      for (auto &I : RangeToBeRemoved) {
----------------
I think there's another case we could detect and warn - that is range start/end is not on instruction boundary. That could happen when perf and binary mismatches. I ran into that couple times. 

We could also check stack sample addresses are on instruction boundary for CS profile, and warn about potential mismatch perf profile. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1052
+      }
+      for (auto &I : RangeToBeRemoved) {
+        Ranges.erase(I);
----------------
I think you meant to place this loop outside of the inner most loop? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111902