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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:283
+  // Assume 4K pagesize for the machine from which the profile has been
+  // collected. This should be fine for now, in case we want to support other
+  // pagesizes it can be recorded in the raw profile during collection.
----------------
Is this limitation going to affect us?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:291
+        // Segment will always be loaded at a page boundary.
+        PreferredTextSegmentAddress = Phdr.p_vaddr & ~(PageSize - 1U);
+        NumExecutableSegments++;
----------------
Is it expected that p_vaddr will not be page boundary aligned already?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:297
+  if (NumExecutableSegments > 1)
+    return report(
+        make_error<StringError>(
----------------
I think it might be clearer to report this error in the above loop. Otherwise when reading that code it is a little confusing why PreferredTextSegmentAddress will be overwritten for each executable segment. Similar suggestion when setting ProfiledTextSegment{Start,End} in setupForSymbolization


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:576
+    const uint64_t AdjustedAddress =
+        VirtualAddress + PreferredTextSegmentAddress - ProfiledTextSegmentStart;
+    return object::SectionedAddress{AdjustedAddress};
----------------
I'm a little confused on the difference between PreferredTextSegmentAddress and ProfiledTextSegmentStart, and the arithmetic being done here. Can you clarify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146181