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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
