[PATCH] D113727: [llvm-profgen] Add switch to allow use of first loadable segment for calculating offset

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:712
+
+      if (!UseOffset || (UseOffset && UseLoadableSegmentAsBase)) {
+        Start = Binary->offsetToVirtualAddr(Start);
----------------
wlei wrote:
> wlei wrote:
> > Wondering if we can avoid changing the code here.
> > 
> > I'm thinking like if we can refactor all `getPreferredBaseAddress` to `getBaseAddress()` or a new function.
> > 
> > Then we have code early in the binary to `setBaseAddress` like:
> > ```
> > if (UseLoadableSegmentAsBase)
> >    setBaseAddress(getFirstLoadableAddress())
> > else
> >    setBaseAddress(getPreferredBaseAddress())
> > ```
> > 
> > Then here `Binary->offsetToVirtualAddr(..);` will cover all the offset cases.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> I realized we might not change `getPreferredBaseAddress ` to `getFirstLoadableAddress` for disassembling, all the CallOffsets, RetOffsets,.. are based on that.  Here It's only for the unsymbolized profile, should be fine. Feel free to ignore this comment.
That will make the definition of base address a bit inconsistent. The base address is supposed to be the address that aligns with mmap base, and we leverage that assumption. See code below.

```
// Drop the event if its image is loaded at the same address
  if (Event.Address == Binary->getBaseAddress()) {
    Binary->setIsLoadedByMMap(true);
    return;
  }
```

If we change base address for binary, while it make this translation easier, it could break mmap matching. 

I'm leaning towards keep this as special case, because the offset computation is a bit weird and we do it really only for compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113727