[PATCH] D103178: [CSSPGO][llvm-profgen] Allow multiple executable load segments.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:332-334
+    const auto &Offsets = Binary.getExeSegmentOffsets();
+    auto It = std::lower_bound(Offsets.begin(), Offsets.end(), Event.Offset);
+    if (It != Offsets.end() && *It == Event.Offset) {
----------------
This could be simplified if we store ExeSegmentOffsets as a std::set<uint64_t>, and let getExeSegmentOffsets return std::set<uint64_t>&?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:349
+        assert(*It < Event.Offset);
+        if (Event.Offset - *It != Event.BaseAddress - Binary.getBaseAddress())
+          exitWithError("Segment not loaded by consecutive mmaps");
----------------
another nit: we named MMapEvent::BaseAddress based on the assumption of single text segment and single load, in which case the address is always the base load address of the binary. now it turns out that's not always the case, perhaps Event::MappedAddress is more appropriate? 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:105
+  // The preferred base address of each executable segment.
+  std::vector<uint64_t> PreferredBaseAddresses;
+  // The file offset of each executable segment.
----------------
nit: I think PreferredBaseAddress is generally about binary load address. Here perhaps we should call it PreferredTextSegmentAddresses? Same for helpers. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:107
+  // The file offset of each executable segment.
+  std::vector<uint64_t> ExeSegmentOffsets;
+
----------------
ExeSegment -> TextSegment? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103178