[PATCH] D118177: [llvm-profgen] Decouple artificial branch from LBR parser and fix external address related issues

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:493
+  // as a call from external.
+  bool isCallFromExternal(UnwindState &State) const {
+    return isSourceExternal(State) &&
----------------
wenlei wrote:
> Check isValidState first.
It's already checked in the only caller,  see `isCallState`.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:510
 
   bool isReturnState(UnwindState &State) const {
     if (!isValidState(State))
----------------
wenlei wrote:
> Good that we don't need to check for artificial return, but perhaps we could assert that the target is next to a call for internal targets? 
Not sure I fully get your point, we have the code in `unwindReturn` which call `getCallAddrFromFrameAddr` to get the target's next call inst. If the return variable(`CallAddr`) is not a call inst, it just return 0. Do you mean to assert on this zero value, I guess currently it won't pass for many cases. I think before we intent to silently give a zero, then the call stack is truncated by this wrong match. 


```
void VirtualUnwinder::unwindReturn(UnwindState &State) {
  // Add extra frame as we unwind through the return
  const LBREntry &LBR = State.getCurrentLBR();
  uint64_t CallAddr = Binary->getCallAddrFromFrameAddr(LBR.Target);
  State.switchToFrame(CallAddr);
  ...
```


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:81
   // isn't a real call context the compiler will see.
-  ExternalAddr = 1,
+  ExternalAddr = 0x1000000000000000ULL,
 };
----------------
wenlei wrote:
> wlei wrote:
> > hoy wrote:
> > > This could be a potential legal address. Use UINT64_MAX instead?
> > Okay, Before I didn't use UINT64_MAX because there can be add operations to address value and adding value to UINT64_MAX caused overflow, we might always keep in mind of this. I changed to UINT64_MAX and fixed one overflow issue, also wondering if we have a better number for this so that overflow can't be issues in the future.
> Technically, both and 1 and UINT64_MAX can be legal address, but practically that probably will never happen. While overflow is an issue with UINT64_MAX, I think 1 is fine?
This is the address not offset, it will be minus the base address, this is more common than "add" overflow. Both 1 and UINT64_MAX can overflow, that's why I choose the value in the middle 0x1000000000000000ULL. But I'm fine with either value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118177