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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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");
----------------
wenlei wrote:
> 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? 
`Event::MappedAddress` sounds good to me, or how about name it just `Address`?


```
  struct MMapEvent {
    uint64_t PID = 0;
    uint64_t BaseAddress = 0;
    uint64_t Size = 0;
    uint64_t Offset = 0;
    StringRef BinaryPath;
  };
```



================
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.
----------------
wenlei wrote:
> nit: I think PreferredBaseAddress is generally about binary load address. Here perhaps we should call it PreferredTextSegmentAddresses? Same for helpers. 
`PreferredTextSegmentAddresses` sounds better.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103178