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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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:
> > > 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.
> >  
> > 
> > so the offset is not the offset as we defined before. However, even it's "wrong", 
> 
> Could the "wrong" offsets now cause trouble for pseudo probes which always have the "correct" offset? It should work fine previously?
> 
> 
> > Yeah, but if we won't use the old tool anymore, I think we can remove all the UseLoadableSegmentAsBase code.
> 
> Agreed.
> 
I looked a bit deeper. The "wrong" offsets should work. `getFirstLoadableAddress` stands for the preferred first load address, not the runtime one where I got the impression from its definition. 

     // The runtime base address that the first loadabe segment is loaded at.
     uint64_t FirstLoadableAddress = 0;


Then we no longer need to remove the feature in this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126827