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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> 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?

> 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?



================
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++;
----------------
Can we hide all this complexity into `getCallAddrFromFrameAddr`? i.e. we could use `bool getCallAddrFromFrameAddr(uint64_t FrameAddr, uint64_t &CallAddr)`. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:529
+      if (!FrameAddr || !Binary->addressIsCall(FrameAddr)) {
+        NumStackSamplesWithInvalidReturnAddress++;
+        break;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109638