Addressing reviewers

Author: paschalis-mpeis

bolt/include/bolt/Profile/DataAggregator.h

@@ -170,6 +170,9 @@ class DataAggregator : public DataReader {
   std::string BuildIDBinaryName;
 
   /// Memory map info for a single file as recorded in perf.data
+  /// When a binary has multiple text segments, the Size is computed as the
+  /// difference of the last address of these segments from the BaseAddress.
+  /// The base addresses of all text segments must be the same.
   struct MMapInfo {
     uint64_t BaseAddress{0}; /// Base address of the mapped binary.
     uint64_t MMapAddress{0}; /// Address of the executable segment.

bolt/lib/Profile/DataAggregator.cpp

@@ -471,7 +471,7 @@ void DataAggregator::filterBinaryMMapInfo() {
 int DataAggregator::prepareToParse(StringRef Name, PerfProcessInfo &Process,
                                    PerfProcessErrorCallbackTy Callback) {
   if (!opts::ReadPerfEvents.empty()) {
-    dbgs() << "PERF2BOLT: using pre-processed perf events for '" << Name
+    outs() << "PERF2BOLT: using pre-processed perf events for '" << Name
            << "' (perf-script-events)\n";
     ParsingBuf = opts::ReadPerfEvents;
     return 0;
@@ -2124,10 +2124,14 @@ std::error_code DataAggregator::parseMMapEvents() {
       MMapInfo.BaseAddress = *BaseAddress;
     }
 
-    // Try to add MMapInfo to the map and update its size. Large binaries
-    // may span multiple text segments, so the mapping is inserted only on the
-    // first occurrence. If a larger section size is found, it will be updated.
-    BinaryMMapInfo.insert(std::make_pair(MMapInfo.PID, MMapInfo));
+    // Try to add MMapInfo to the map and update its size. Large binaries may
+    // span to multiple text segments, so the mapping is inserted only on the
+    // first occurrence.
+    if (!BinaryMMapInfo.insert(std::make_pair(MMapInfo.PID, MMapInfo)).second)
+      assert(MMapInfo.BaseAddress == BinaryMMapInfo[MMapInfo.PID].BaseAddress &&
+             "Base address on multiple segment mappings should match");
+
+    // Update section size.
     uint64_t EndAddress = MMapInfo.MMapAddress + MMapInfo.Size;
     uint64_t Size = EndAddress - BinaryMMapInfo[MMapInfo.PID].BaseAddress;
     if (Size > BinaryMMapInfo[MMapInfo.PID].Size)

bolt/unittests/Core/MemoryMaps.cpp

@@ -25,6 +25,10 @@ extern cl::opt<std::string> ReadPerfEvents;
 } // namespace opts
 
 namespace {
+
+/// Perform checks on memory map events normally captured in perf. Tests use
+/// the 'opts::ReadPerfEvents' flag to emulate these events, passing a custom
+/// 'perf script' output to DataAggregator.
 struct MemoryMapsTester : public testing::TestWithParam<Triple::ArchType> {
   void SetUp() override {
     initalizeLLVM();
@@ -81,8 +85,7 @@ INSTANTIATE_TEST_SUITE_P(AArch64, MemoryMapsTester,
 #endif
 
 /// Check that the correct mmap size is computed when we have multiple text
-/// segment mappings. Uses 'opts::ReadPerfEvents' flag to pass a custom 'perf
-/// script' output, along with two text segments (SegmentInfo).
+/// segment mappings.
 TEST_P(MemoryMapsTester, ParseMultipleSegments) {
   const int Pid = 1234;
   StringRef Filename = "BINARY";
@@ -94,13 +97,9 @@ TEST_P(MemoryMapsTester, ParseMultipleSegments) {
       Pid, Filename);
 
   BC->SegmentMapInfo[0x11da000] =
-      SegmentInfo{0x11da000, 0x10da000, 0x11ca000, 0x10da000, 0x10000};
+      SegmentInfo{0x11da000, 0x10da000, 0x11ca000, 0x10da000, 0x10000, true};
   BC->SegmentMapInfo[0x31d0000] =
-      SegmentInfo{0x31d0000, 0x51ac82c, 0x31d0000, 0x3000000, 0x200000};
-
-  // Dont show DataAggregators out/err output.
-  testing::internal::CaptureStdout();
-  testing::internal::CaptureStderr();
+      SegmentInfo{0x31d0000, 0x51ac82c, 0x31d0000, 0x3000000, 0x200000, true};
 
   DataAggregator DA("");
   BC->setFilename(Filename);
@@ -115,3 +114,29 @@ TEST_P(MemoryMapsTester, ParseMultipleSegments) {
   ASSERT_NE(El, BinaryMMapInfo.end());
   ASSERT_EQ(El->second.Size, static_cast<uint64_t>(0xb1d0000));
 }
+
+/// Check that DataAggregator aborts when pre-processing an input binary
+/// with multiple text segments that have different base addresses.
+TEST_P(MemoryMapsTester, MultipleSegmentsMismatchedBaseAddress) {
+  const int Pid = 1234;
+  StringRef Filename = "BINARY";
+  opts::ReadPerfEvents = formatv(
+      "name       0 [000]     0.000000: PERF_RECORD_MMAP2 {0}/{0}: "
+      "[0xabc0000000(0x1000000) @ 0x11c0000 103:01 1573523 0]: r-xp {1}\n"
+      "name       0 [000]     0.000000: PERF_RECORD_MMAP2 {0}/{0}: "
+      "[0xabc2000000(0x8000000) @ 0x31d0000 103:01 1573523 0]: r-xp {1}\n",
+      Pid, Filename);
+
+  BC->SegmentMapInfo[0x11da000] =
+      SegmentInfo{0x11da000, 0x10da000, 0x11ca000, 0x10da000, 0x10000, true};
+  // Using '0x31d0fff' FileOffset which triggers a different base address
+  // for this second text segment.
+  BC->SegmentMapInfo[0x31d0000] =
+      SegmentInfo{0x31d0000, 0x51ac82c, 0x31d0fff, 0x3000000, 0x200000, true};
+
+  DataAggregator DA("");
+  BC->setFilename(Filename);
+  ASSERT_DEATH(
+      { Error Err = DA.preprocessProfile(*BC); },
+      "Base address on multiple segment mappings should match");
+}