This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profgen] Add switch to allow use of first loadable segment for calculating offset
ClosedPublic

Authored by wenlei on Nov 11 2021, 6:30 PM.

Details

Summary

Adding -use-loadable-segment-as-base to allow use of first loadable segment for calculating offset. By default first executable segment is used for calculating offset. The switch helps compatibility with unsymbolized profile generated from older tools.

Diff Detail

Event Timeline

wenlei created this revision.Nov 11 2021, 6:30 PM
wenlei requested review of this revision.Nov 11 2021, 6:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2021, 6:30 PM
hoy added inline comments.Nov 12 2021, 9:10 AM
llvm/tools/llvm-profgen/PerfReader.cpp
32

Name it use-first-loadable-segment-as-base?

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

nit: add a const qualifier

wenlei updated this revision to Diff 386871.Nov 12 2021, 9:23 AM

address comment

hoy accepted this revision.Nov 12 2021, 9:27 AM

lgtm, thanks

This revision is now accepted and ready to land.Nov 12 2021, 9:27 AM
wlei added inline comments.Nov 12 2021, 9:43 AM
llvm/tools/llvm-profgen/PerfReader.cpp
713

Wondering if we can avoid changing the code here.

I'm thinking like if we can refactor all getPreferredBaseAddress to getBaseAddress() or a new function.

Then we have code early in the binary to setBaseAddress like:

if (UseLoadableSegmentAsBase)
   setBaseAddress(getFirstLoadableAddress())
else
   setBaseAddress(getPreferredBaseAddress())

Then here Binary->offsetToVirtualAddr(..); will cover all the offset cases.

wlei accepted this revision.Nov 12 2021, 11:07 AM

LGTM, thanks for the change!

llvm/tools/llvm-profgen/PerfReader.cpp
713

I realized we might not change getPreferredBaseAddress to getFirstLoadableAddress for disassembling, all the CallOffsets, RetOffsets,.. are based on that. Here It's only for the unsymbolized profile, should be fine. Feel free to ignore this comment.

wenlei added inline comments.Nov 13 2021, 6:52 PM
llvm/tools/llvm-profgen/PerfReader.cpp
713

That will make the definition of base address a bit inconsistent. The base address is supposed to be the address that aligns with mmap base, and we leverage that assumption. See code below.

// Drop the event if its image is loaded at the same address
  if (Event.Address == Binary->getBaseAddress()) {
    Binary->setIsLoadedByMMap(true);
    return;
  }

If we change base address for binary, while it make this translation easier, it could break mmap matching.

I'm leaning towards keep this as special case, because the offset computation is a bit weird and we do it really only for compatibility.

wlei added inline comments.Nov 15 2021, 10:14 AM
llvm/tools/llvm-profgen/PerfReader.cpp
713

That makes sense, thanks for the clarification!

This revision was landed with ongoing or failed builds.Nov 15 2021, 7:02 PM
This revision was automatically updated to reflect the committed changes.