[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:120-132
+  if (LBR.IsArtificial && CallAddr == 0) {
+    // In a callback case, a return from internal code, say A, to external
+    // runtime can happen. The external runtime can then call back to
+    // another internal routine, say B. Making an artificial branch that
+    // looks like a return from A to B can confuse the unwinder to treat
+    // the instruction before B as the call instruction. Here we detect this
+    // case if the return target is not the next inst of call inst, then we just
----------------
Can we move this check into `isReturnState`? This should not be considered as return state, then we would naturally fall back to `unwindBranchWithinFrame` which does what we want. We may need to rename `unwindBranchWithinFrame` to simply `unwindBranch` because this now can be inter-function too. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:298
 
-  void pushFrame(uint64_t Address) {
+  void pushFrame(uint64_t Address, bool IsArtificial = false) {
+    // Push an artificial frame for the case of returning to external address
----------------
Comment for the definition and purpose for artificial frame.


================
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);
----------------
nit: if we don't expect pushing artificial frame, the message can be explicit, something like "Artificial frame not expected for context stack."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115550