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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:712
+
+      if (!UseOffset || (UseOffset && UseLoadableSegmentAsBase)) {
+        Start = Binary->offsetToVirtualAddr(Start);
----------------
wenlei wrote:
> 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.
That makes sense, thanks for the clarification!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113727