[PATCH] D115550: [CSSPGO][llvm-profgen] Fix external address issues of perf reader (return to external addr part)

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:147
   State.switchToFrame(CallAddr);
+  // Push an artificial frame(external address) for the case of returning to
+  // external address(callback), later if an aitificial call is matched and it
----------------
replace "artificial frame" with "external frame" now that we're converging on the term? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:528-530
+    uint64_t CallAddr =
+        Binary->getCallAddrFromFrameAddr(State.getCurrentLBRTarget());
+    return CallAddr;
----------------
nit: `return (CallAddr != 0);` to make the boolean return explicit. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:433
   bool pushFrame(UnwindState::ProfiledFrame *Cur) {
+    assert(!Cur->IsArtificialFrame() && "Push an artificial frame.");
     Stack.push_back(Cur->Address);
----------------
wlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > nit: if we don't expect pushing artificial frame, the message can be explicit, something like "Artificial frame not expected for context stack."
> > Or we just return false for artificial frames so that it doesn't need to be handled on the caller side?
> Sounds good, changed to return false for external frame.
I actually think that it's cleaner to handle this on caller side and assert we don't have external frame when it comes here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115550