Page MenuHomePhabricator

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

Authored by wlei on Jun 1 2022, 2:16 PM.

Details

Reviewers
hoy
wenlei
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.

Diff Detail

Event Timeline

wlei created this revision.Jun 1 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 2:16 PM
Herald added subscribers: hoy, wenlei. · View Herald Transcript
wlei requested review of this revision.Jun 1 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 2:16 PM
wlei retitled this revision from [llvm-profgen] fix a loading address bug for pseudo probe profile to [llvm-profgen] Fix a loading address bug for pseudo probe profile.Jun 1 2022, 2:21 PM
wlei edited the summary of this revision. (Show Details)
wlei added reviewers: hoy, wenlei.
wenlei added inline comments.Jun 8 2022, 12:08 AM
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 added inline comments.Jun 8 2022, 10:53 AM
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 added inline comments.Jun 8 2022, 2:30 PM
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 added inline comments.Jun 8 2022, 2:39 PM
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 added inline comments.Jun 8 2022, 5:48 PM
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 updated this revision to Diff 440317.Jun 27 2022, 11:00 AM

changed all offset usage to preferred address based virtual address

hoy added a comment.Jul 8 2022, 4:26 PM

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 added inline comments.Mon, Jul 11, 4:20 PM
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 added inline comments.Wed, Jul 13, 11:33 AM
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 added inline comments.Wed, Jul 13, 4:40 PM
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 added inline comments.Wed, Jul 13, 5:05 PM
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 added inline comments.Wed, Jul 13, 5:24 PM
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.

hoy accepted this revision.Thu, Jul 14, 9:52 AM
This revision is now accepted and ready to land.Thu, Jul 14, 9:52 AM