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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


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


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:62
   auto *ParentFrame = State.getParentFrame();
+  if (ParentFrame->Address != Source && State.isLeadingFrameExternal())
+    NumMismatchedFrame++;
----------------
Actually do we need the check on `isLeadingFrameExternal`? Should we always expect parent frame address and LBR source to align for any calls? 

Additionally, should there a one instruction shift between frame address and source/call address (see `getCallAddrFromFrameAddr`)?

And when mismatch happens, should we abort unwinding? It seems that at this point it's guaranteed to generate incorrect context? 

How you seen such mismatch happening? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:242
   UnwindState State(Sample, Binary);
+  NumTotalLBR += State.getLBRStackSize();
 
----------------
nit: this is more like total branches. usually LBR record is considered the full 16/32 entires of branch pairs in LBR. same applies to the warning message. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:275-276
     } else {
       // Unwind branches - for regular intra function branches, we only
       // need to record branch with context.
+      unwindBranch(State);
----------------
Update the comment as it's no longer accurate - it could regular intra function branch, or artificial branch cross function boundaries when there're external functions involved (example would be good). 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:302
   void advanceLBR() { LBRIndex++; }
+  bool isLeadingFrameExternal() const {
+    if (!LBRStack.empty() && LBRStack[0].Target == ExternalAddr)
----------------
nit: LeadingFrame -> LeafFrame. LeafLBR -> LastLBR. Leaf or root is used for frame, but there's no leaf or root for LBR, which isn't hierarchical. If there're other inconsistent usage of the terms, we can fix them too.  

Additionally, why not check on CurrentLeafFrame instead given the function name says "Frame"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115538