This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profgen] Fix inconsistent loading address issues
ClosedPublic

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

Details

Summary

This is to fix two issues related with loading address:

  1. When multiple MMAPs occur and their loading address are different, before it only used the first MMap as base address, all perf address after it used the wrong base address.
  1. For pseudo probe profile, the 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.

Solution: Instead of converting the address to offset lazily, right now all the address after parsing are converted on the fly based on preferred loading address in the parsing time. There is no "offset" used in profile generator any more.

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.Jul 11 2022, 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.Jul 13 2022, 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.Jul 13 2022, 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.Jul 13 2022, 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.Jul 13 2022, 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.Jul 14 2022, 9:52 AM
This revision is now accepted and ready to land.Jul 14 2022, 9:52 AM
wenlei accepted this revision.Oct 13 2022, 5:07 PM

Agreed that having everything canonicalized to use preferred load address as base is clean and practical. Thanks for making the changes. LGTM with a nit.

llvm/tools/llvm-profgen/ProfiledBinary.h
353

nit: canonicalizeVirtualAddress is probably a better name.

wlei updated this revision to Diff 467684.Oct 13 2022, 10:57 PM

rebase and rename convertAddress --> canonicalizeVirtualAddress

wlei retitled this revision from [llvm-profgen] Fix a loading address bug for pseudo probe profile to [llvm-profgen] Fix inconsistent loading address issues.Oct 13 2022, 11:15 PM
wlei edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Oct 13 2022, 11:25 PM
This revision was automatically updated to reflect the committed changes.