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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:624
+        NumTopExternalFrame++;
+        CallStack.emplace_back(UnwindState::ProfiledFrame::ExternalAddr);
+      }
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > hoy wrote:
> > > > wlei wrote:
> > > > > wenlei wrote:
> > > > > > If only leaf frames are external, can we keep the remaining frames for unwinding?
> > > > > Sounds good, tried to keep the remaining internal call stack for unwinding.
> > > > Maybe we can keep the whole call stack by replacing consecutive external frames with an artificial frame introduced in the other patch. This should be synchronized with the LBR parser where we replace consecutive external LBRs with an artificial branch. 
> > > Good point. Changed to both use `external branch`, update in the next patch.
> > Thanks. Just to be clear, we will no longer need to break, instead, we'll continue whenever the top of stack is also an ExternalAddr. This way we could handle the following case
> > 
> > 
> > ```
> > Ext
> > Ext
> > foo
> > Ext
> > Ext
> > bar
> > main
> > Ext/Ext /Ext/Ext foo/Ext foo/foo Ext/foo Ext/Ext bar/Ext bar/bar bar/bar ...
> > ```
> > 
> > Though this should be rare, for completeness does it sounds reasonable? 
> > 
> That's a good point. Only one thing: we don't know `Ext/Ext` is a Call or Return or Jmp. So I think we should preprocess to deduplicate the Ext in the call stack. like we also deduplicate the Ext in LBRsample.
> in your case, we will get.
> ```
> Ext
> Ext
> foo
> Ext
> Ext
> bar
> main
> ```
> to 
> 
> ```
> Ext
> foo
> Ext
> bar
> main
> ```
> 
Exactly, that is actually what I meant, i.e, to skip the current Ext frame only if the top of stack is already an Ext. 

Do you think we need the other change to correctly truncate such as mixed callsite when reporting samples? Sounds good to implement there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115538