[PATCH] D111902: [llvm-profgen] Warn on invalid range and show warning summary

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1038
+    return;
+  WithColor::warning() << format("%.2f", static_cast<double>(Num) * 100 / Total)
+                       << "%(" << Num << "/" << Total
----------------
wenlei wrote:
> Suggest we rephrase the message.
> 
> ```
> warning: 0.05%(1120/2428958) cases with issue: Profile context truncated due to missing probe for call instruction.
> ```
> ->
> ```
> warning: 0.05% (1120/2428958) of profiled contexts are truncated due to missing probe for call instruction.
> ```
> 
> ```
> warning: 0.00%(2/178637) cases with issue: Range does not belong to any functions, likely from external function.
> ```
> ->
> ```
> warning: 0.00% (2/178637) of profiled ranges do not belong to any functions.
> ```
> 
> And specifically for invalid ranges, I thought we filter out external entries already, no? Did you check that they are actually from external functions?
We filter out the address that's not in the ELF text section(false for `addressIsCode`), I guess we name that the external entry.

Here the two((2/178637)) "external" functions is still inside ELF text section, like the PLT, `.init`, `.fini`  section. It's also meaningless I think, you see the num is very small.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1097
+
+    auto *FRange = Binary->findFuncRangeForOffset(StartOffset);
+    if (!FRange) {
----------------
wenlei wrote:
> could also check `addressIsCode(StartOffset)` to make sure start is also on instruction boundary? 
 Seems we call `addressIsCode` in early time in parseSample, so if that works well, we don't need to call here. But I guess you mean to detect  the bugs here. added `addressIsCode `.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1099
+    if (!FRange) {
+      DanglingRange++;
+      WarnInvalidRange(StartOffset, EndOffset, DanglingRangeMsg);
----------------
wenlei wrote:
> "dangling" can be a confusing term. If this is truly external, we can say ExternalRange, or whatever that's more explicit.
It means it doesn't find/match the function range in ELF, maybe not external, how about `UnmatchedRange`?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:327
 
+  bool offsetIsBoundry(uint64_t Offset) {
+    return BranchOffsets.count(Offset) || RetOffsets.count(Offset) ||
----------------
wenlei wrote:
> nit: offsetIsTransfer - this would be consistent with others. flow transfer instructions includes call,ret,branch.
Fixed, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111902