[PATCH] D115538: [CSSPGO][llvm-profgen] Fix external address issues of perf reader (leading external LBR part)

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks.



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:261
+    if (IsLeaf && State.CurrentLeafFrame->isExternalFrame()) {
+      State.InstPtr.update(State.getCurrentLBRSource());
+      State.switchToFrame(State.getCurrentLBRSource());
----------------
wlei wrote:
> wenlei wrote:
> > wlei wrote:
> > > wenlei wrote:
> > > > I think we can do some sanity check here:
> > > > 1. assert that we have a paired LBR here? ie. LBR target is external.
> > > > 2. also assert that the next frame on stack aligns with LBR source?
> > > >assert that we have a paired LBR here? ie. LBR target is external.
> > > Sounds good, added assertion in `validateInitialState`.
> > > 
> > > > also assert that the next frame on stack aligns with LBR source?
> > > 
> > > If I understand correctly, that assumes the LBR source should be a CALL? The tricky part is we don't only have CALL into external address, we can have RETURN, JMP.  One common cases is the PLT,  internal call --> PLT--> external address. PLT code is a JMP.
> > > 
> > > How about to give a warning if the following CALL unwinding hit a mismatched frame.
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > On 2nd thought, you're right that if the transfer is not a call, there will be mismatch, PLT is one, tail call jump can be another. 
> > 
> > Thought for PLT, perhaps we can treat PLT as external too, so anything involved PLT will be stripped too.
> > 
> > I was hoping that we can have a sense of how intrusive this kind of mismatch is. Hopefully it's rare. Warning is good but perhaps PLT case can be handled? 
> PLT case can be handled, we can have a diff to make PLT code as external code. then we can have some statistic base on this. I will have a separated diff for PLT stuffs.
Dealing with PLT in separate patch sounds good. And I guess we only need to deal with it if it's important enough. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115538