[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:1195
+  // preferred loading address in order to match pseduo probe address correctly.
+  Binary->setBaseAddress(Binary->getPreferredBaseAddress());
+
----------------
hoy wrote:
> wenlei wrote:
> > 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. 
> 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?
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`




 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:374
+// Offset-based context id
+struct OffsetBasedCtxKey : public ContextKey {
   SmallVector<uint64_t, 16> Context;
----------------
wenlei wrote:
> 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..
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126827