[PATCH] D146181: [memprof] Support symbolization of PIE binaries.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm. Couple suggestions for asserts if you think they make sense.



================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:291
+        // Segment will always be loaded at a page boundary.
+        PreferredTextSegmentAddress = Phdr.p_vaddr & ~(PageSize - 1U);
+        NumExecutableSegments++;
----------------
snehasish wrote:
> tejohnson wrote:
> > Is it expected that p_vaddr will not be page boundary aligned already?
> It most likely will be page (or 2M hugepage aligned) already. The default is to align by 4K [1] and this should almost always be a no-op since even 2M aligned is also 4K aligned.
> 
> 
> [1] https://github.com/llvm/llvm-project/blob/main/lld/ELF/Target.h#L112
Consider making this an assert that it is already page aligned?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:576
+    const uint64_t AdjustedAddress =
+        VirtualAddress + PreferredTextSegmentAddress - ProfiledTextSegmentStart;
+    return object::SectionedAddress{AdjustedAddress};
----------------
snehasish wrote:
> tejohnson wrote:
> > I'm a little confused on the difference between PreferredTextSegmentAddress and ProfiledTextSegmentStart, and the arithmetic being done here. Can you clarify?
> For non-PIE binaries:
> PreferredTextSegmentAddress is non-zero, e.g 0x200000 or 0x400000. In this case the loader will put the segment at the virtual address requested by the binary and the ProfiledTextSegmentStart will be the same as PreferredTextSegmentAddress. Thus this arithmetic is a no-op.
> 
> For PIE binaries:
> PreferredTextSegmentAddress is zero. The loader is free to place the segment at any address (respecting alignment specified by the header). Then the instruction offset in the binary is computed as the virtual address minus the start address of the ProfiledTextSegment. This assumes p_offset = 0 for the executable segment (added an assertion).
> 
> Added comments to the code to clarify.
Thanks, the new comment helps. Perhaps add an assert somewhere like:
assert(PreferredTextSegmentAddress == 0 || (PreferredTextSegmentAddress == ProfiledTextSegmentStart));


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146181