[PATCH] D113727: [llvm-profgen] Add switch to allow use of first loadable segment for calculating offset
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Nov 13 18:52:19 PST 2021
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
More information about the llvm-commits
mailing list