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

[wlei]

Summary

Base address can be loaded from MMP event, but pseudo probe address is always based on preferred loading address. If the base address is not equal to the preferred loading address, the pseudo probe address query will be wrong. Recover the base address to preferred loading address in order to match pseudo probe address correctly.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

1199

Where do we use getBaseAddress in probe queries? Can we use getPreferredBaseAddress directly there? Ideally there should be one definition for base address which is the actual executable segment load address, and changing it here makes it inconsistent.

llvm/tools/llvm-profgen/PerfReader.h

374 ↗

(On Diff #433547)

Perhaps AddrBasedCtxKey as name is just fine? This is in contrast with StringBasedCtxKey, so Addr as a general term isn't a big deal, I don't have strong opinion though..

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

1199

Agreed that setBaseAddress should be called once to be consistent. Pseudo probe decoding is based on the preferred address. I think a reasonable fix could be

1. make the decoding offset-based. This will require a change to the bolt code. base; or
2. do an offset to preferred addr translation for every probe look up in llvm-profgen. This is being done in some places, eg, extractPrefixContextStack.

#2 sounds more practical. WDYT?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

1199

Hmm, I found it's very tricky case.

Where do we use getBaseAddress in probe queries?

We call it in Binary->offsetToVirtualAddr.

However, we call IP.advance() to iterate all the address in the range, in IP it will convert the Address back to offset using the base address. like Offset + preferred Address - Mmap loading address. That's another inconsistency.. Then we need to change code inside of the IP. but IP also used in PerfReader

llvm/tools/llvm-profgen/PerfReader.h

374 ↗

(On Diff #433547)

Sounds good.

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

1199

Or can we use virtual address(Offset + preferred Address) for all the code(PerfReader and ProfileGenerator)? to do this, we can convert the address at the very beginning of PerfReader. like Address = PhysicalAddress - Mmap loading address + PreferredAddress

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

1199

I see. This is complicated. How about we always use preferred address when doing this offset to virtual addr conversion, and we rebase all LBR and stack sample addresses to be on the preferred load address in the the perf reader ?

[wlei]

changed all offset usage to preferred address based virtual address

[hoy]

In D126827#3613023, @wlei wrote:

changed all offset usage to preferred address based virtual address

Thanks for the work! It should be more reliable and also looks cleaner.

llvm/tools/llvm-profgen/PerfReader.cpp

745

I'm a bit confused here. What is the semantics of using both offset and LoadableSegmentAsBase?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

745

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.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

745

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.

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

745

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.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

745

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.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

745

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.