Page MenuHomePhabricator

[CSSPGO][llvm-profgen] Truncate stack samples with invalid return address.
ClosedPublic

Authored by hoy on Fri, Sep 10, 5:33 PM.

Details

Summary

Invalid frame addresses exist in call stack samples due to bad unwinding. This could happen to frame-pointer-based unwinding and the callee functions that do not have the frame pointer chain set up. 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.

Diff Detail

Event Timeline

hoy created this revision.Fri, Sep 10, 5:33 PM
hoy requested review of this revision.Fri, Sep 10, 5:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFri, Sep 10, 5:33 PM
hoy retitled this revision from [CSSPGO] Enable pseudo probe instrumentation in O0 mode. to [CSSPGO] Truncate stack samples with invalid return address..Fri, Sep 10, 5:40 PM
hoy edited the summary of this revision. (Show Details)
hoy updated this revision to Diff 372042.Fri, Sep 10, 5:49 PM
hoy edited the summary of this revision. (Show Details)

Updating D109638: [CSSPGO] Truncate stack samples with invalid return address.

hoy retitled this revision from [CSSPGO] Truncate stack samples with invalid return address. to [CSSPGO][llvm-profgen] Truncate stack samples with invalid return address..Tue, Sep 14, 9:23 AM

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?

llvm/tools/llvm-profgen/PerfReader.cpp
524–533

Can we hide all this complexity into getCallAddrFromFrameAddr? i.e. we could use bool getCallAddrFromFrameAddr(uint64_t FrameAddr, uint64_t &CallAddr).

530

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.

hoy added a comment.Tue, Sep 14, 6:01 PM

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.

llvm/tools/llvm-profgen/PerfReader.cpp
524–533

Sounds good to move it into getCallAddrFromFrameAddr.

530

Deduplicated warnings added.

hoy updated this revision to Diff 372604.Tue, Sep 14, 6:03 PM

Addressing Wenlei's comment.

wenlei accepted this revision.Tue, Sep 14, 6:27 PM

lgtm except two nits.

llvm/tools/llvm-profgen/PerfReader.cpp
772

nit: emitAccumulatedWarnings -> warnTruncatedStack

774

Nit: make the message more meaningful. Truncated stack sample due to invalid return address at 0xabcd, likely caused by frame pointer omission.

This revision is now accepted and ready to land.Tue, Sep 14, 6:27 PM
hoy updated this revision to Diff 372615.Tue, Sep 14, 6:44 PM

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

llvm/tools/llvm-profgen/PerfReader.cpp
772

Makes sense. I was making this cover all warnings for future use. But sounds good to stick with truncated stack for now.

This revision was landed with ongoing or failed builds.Tue, Sep 14, 9:56 PM
This revision was automatically updated to reflect the committed changes.