[PATCH] D126827: [llvm-profgen] Fix a loading address bug for pseudo probe profile

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:745
+        if (UseLoadableSegmentAsBase) {
+          Start -= Binary->getFirstLoadableAddress();
+          End -= Binary->getFirstLoadableAddress();
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > I'm a bit confused here. What is the semantics of using both offset and LoadableSegmentAsBase?
> > I think this is to be compatible to our internal old tool where some services use offset and the FirstLoadableAddress as the output of unsymbolized profile.
> > 
> > The original patch is https://reviews.llvm.org/D113727.
> Do we still need this now? The offset here is computed by subtracting the runtime base address from preferred-based virtual address. This seems different from what previous computed, ie., subtracting the runtime base address from runtime virtual address.
It should be the same behavior as previous offset computation.

This is about how we define the "offset": Assuming MMAP loading address is missing or just equal to  preferred-based-address, then the `virtual address` = `offset + preferred-based-address`. and for this two branches, if it's `UseLoadableSegmentAsBase` is False, then the offset is `virtual address - preferred-based-address` --> `offset +  preferred-based-address - preferred-based-address` which is the original "offset". but when  `UseLoadableSegmentAsBase` is True, the offset is  `virtual address - FirstLoadableAddress` -->`offset +  preferred-based-address - FirstLoadableAddress`
so the offset is not the `offset` as we defined before. However, even it's "wrong", if depends on the source of input, our internal tool which just produced this "wrong" offset, so llvm-profgen also need to use the same way to recover it.

Yeah, but if we won't use the old tool anymore, I think we can remove all the `UseLoadableSegmentAsBase` code.
 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126827