[PATCH] D109638: [CSSPGO][llvm-profgen] Truncate stack samples with invalid return address.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D109638#3000682 <https://reviews.llvm.org/D109638#3000682>, @wenlei wrote:

>> It isn't common when the program is built with the frame pointer omission disabled, but can still happen with third-party static libs built with frame pointer omitted.
>
> Did you mean disabled -> enabled?

It's actually disabled. This happens when the program is built with -fno-omit-frame-pointer but the third-party lib is built with -fomit-frame-pointer.

>> This could happen to frame-pointer-based unwinding and the callee functions that do not have the frame pointer chain set up.
>
> So this leads to frame pointer being used to volatile register and assume it contains frame pointer would lead to bad frame address when unwinding the stack during profiling, is that what you observed?

Exactly. I was seeing `rbp` used as a general register and its value was trashed.



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:523-531
+      auto I = Binary->getIndexForAddr(FrameAddr);
+      FrameAddr = I ? Binary->getAddressforIndex(I - 1) : 0;
+      // Stop at an invalid return address caused by bad unwinding. This could
+      // happen to frame-pointer-based unwinding and the callee functions that
+      // do not have the frame pointer chain set up.
+      if (!FrameAddr || !Binary->addressIsCall(FrameAddr)) {
+        NumStackSamplesWithInvalidReturnAddress++;
----------------
wenlei wrote:
> Can we hide all this complexity into `getCallAddrFromFrameAddr`? i.e. we could use `bool getCallAddrFromFrameAddr(uint64_t FrameAddr, uint64_t &CallAddr)`. 
Sounds good to move it into `getCallAddrFromFrameAddr`.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:529
+      if (!FrameAddr || !Binary->addressIsCall(FrameAddr)) {
+        NumStackSamplesWithInvalidReturnAddress++;
+        break;
----------------
wenlei wrote:
> Should we be using a warning too? For truncated context due to missing probe for merged callsite, we used warning. I think this is things of similar nature, should we also handle them in similar/consistent fashion- i.e. with warning instead of stats? 
> 
> And if we expect duplicated warnings, would be good to deduplicated before printing. 
Deduplicated warnings added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109638