Merge branch 'main' into sdag/arm-glue

Author: s-barannikov

.ci/generate_test_report.py

@@ -18,7 +18,7 @@ def junit_from_xml(xml):
 
 class TestReports(unittest.TestCase):
     def test_title_only(self):
-        self.assertEqual(_generate_report("Foo", []), ("", None))
+        self.assertEqual(_generate_report("Foo", []), ("", "success"))
 
     def test_no_tests_in_testsuite(self):
         self.assertEqual(
@@ -336,7 +336,7 @@ def _generate_report(title, junit_objects, size_limit=1024 * 1024, list_failures
                     )
 
     if not tests_run:
-        return ("", style)
+        return ("", None)
 
     style = "error" if tests_failed else "success"
     report = [f"# {title}", ""]

.github/new-issues-labeler.yml

@@ -30,3 +30,9 @@
 
 'infra:commit-access-request':
   - '/Request Commit Access/'
+
+'false-positive':
+  - '\bfalse[- ]positive\b'
+
+'false-negative':
+  - '\bfalse[- ]negative\b'

bolt/include/bolt/Core/BinaryFunction.h

@@ -2407,6 +2407,19 @@ inline raw_ostream &operator<<(raw_ostream &OS,
   return OS;
 }
 
+/// Compare function by index if it is valid, fall back to the original address
+/// otherwise.
+inline bool compareBinaryFunctionByIndex(const BinaryFunction *A,
+                                         const BinaryFunction *B) {
+  if (A->hasValidIndex() && B->hasValidIndex())
+    return A->getIndex() < B->getIndex();
+  if (A->hasValidIndex() && !B->hasValidIndex())
+    return true;
+  if (!A->hasValidIndex() && B->hasValidIndex())
+    return false;
+  return A->getAddress() < B->getAddress();
+}
+
 } // namespace bolt
 
 // GraphTraits specializations for function basic block graphs (CFGs)

bolt/include/bolt/Profile/DataAggregator.h

@@ -170,9 +170,6 @@ 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.
@@ -496,11 +493,6 @@ class DataAggregator : public DataReader {
   /// and return a file name matching a given \p FileBuildID.
   std::optional<StringRef> getFileNameForBuildID(StringRef FileBuildID);
 
-  /// Get a constant reference to the parsed binary mmap entries.
-  const std::unordered_map<uint64_t, MMapInfo> &getBinaryMMapInfo() {
-    return BinaryMMapInfo;
-  }
-
   friend class YAMLProfileWriter;
 };
 } // namespace bolt

bolt/lib/Core/BinaryContext.cpp

@@ -1607,13 +1607,7 @@ std::vector<BinaryFunction *> BinaryContext::getSortedFunctions() {
                   SortedFunctions.begin(),
                   [](BinaryFunction &BF) { return &BF; });
 
-  llvm::stable_sort(SortedFunctions,
-                    [](const BinaryFunction *A, const BinaryFunction *B) {
-                      if (A->hasValidIndex() && B->hasValidIndex()) {
-                        return A->getIndex() < B->getIndex();
-                      }
-                      return A->hasValidIndex();
-                    });
+  llvm::stable_sort(SortedFunctions, compareBinaryFunctionByIndex);
   return SortedFunctions;
 }
 

bolt/lib/Core/BinaryFunction.cpp

@@ -385,16 +385,7 @@ bool BinaryFunction::isForwardCall(const MCSymbol *CalleeSymbol) const {
   if (CalleeBF) {
     if (CalleeBF->isInjected())
       return true;
-
-    if (hasValidIndex() && CalleeBF->hasValidIndex()) {
-      return getIndex() < CalleeBF->getIndex();
-    } else if (hasValidIndex() && !CalleeBF->hasValidIndex()) {
-      return true;
-    } else if (!hasValidIndex() && CalleeBF->hasValidIndex()) {
-      return false;
-    } else {
-      return getAddress() < CalleeBF->getAddress();
-    }
+    return compareBinaryFunctionByIndex(this, CalleeBF);
   } else {
     // Absolute symbol.
     ErrorOr<uint64_t> CalleeAddressOrError = BC.getSymbolValue(*CalleeSymbol);

bolt/lib/Passes/ReorderFunctions.cpp

@@ -473,16 +473,7 @@ Error ReorderFunctions::runOnFunctions(BinaryContext &BC) {
                     [](BinaryFunction &BF) { return &BF; });
 
     // Sort functions by index.
-    llvm::stable_sort(SortedFunctions,
-                      [](const BinaryFunction *A, const BinaryFunction *B) {
-                        if (A->hasValidIndex() && B->hasValidIndex())
-                          return A->getIndex() < B->getIndex();
-                        if (A->hasValidIndex() && !B->hasValidIndex())
-                          return true;
-                        if (!A->hasValidIndex() && B->hasValidIndex())
-                          return false;
-                        return A->getAddress() < B->getAddress();
-                      });
+    llvm::stable_sort(SortedFunctions, compareBinaryFunctionByIndex);
 
     for (const BinaryFunction *Func : SortedFunctions) {
       if (!Func->hasValidIndex())

bolt/lib/Profile/DataAggregator.cpp

@@ -95,12 +95,6 @@ cl::opt<bool> ReadPreAggregated(
     "pa", cl::desc("skip perf and read data from a pre-aggregated file format"),
     cl::cat(AggregatorCategory));
 
-cl::opt<std::string>
-    ReadPerfEvents("perf-script-events",
-                   cl::desc("skip perf event collection by supplying a "
-                            "perf-script output in a textual format"),
-                   cl::ReallyHidden, cl::init(""), cl::cat(AggregatorCategory));
-
 static cl::opt<bool>
 TimeAggregator("time-aggr",
   cl::desc("time BOLT aggregator"),
@@ -173,9 +167,8 @@ void DataAggregator::findPerfExecutable() {
 void DataAggregator::start() {
   outs() << "PERF2BOLT: Starting data aggregation job for " << Filename << "\n";
 
-  // Don't launch perf for pre-aggregated files or when perf input is specified
-  // by the user.
-  if (opts::ReadPreAggregated || !opts::ReadPerfEvents.empty())
+  // Don't launch perf for pre-aggregated files
+  if (opts::ReadPreAggregated)
     return;
 
   findPerfExecutable();
@@ -471,13 +464,6 @@ void DataAggregator::filterBinaryMMapInfo() {
 
 int DataAggregator::prepareToParse(StringRef Name, PerfProcessInfo &Process,
                                    PerfProcessErrorCallbackTy Callback) {
-  if (!opts::ReadPerfEvents.empty()) {
-    outs() << "PERF2BOLT: using pre-processed perf events for '" << Name
-           << "' (perf-script-events)\n";
-    ParsingBuf = opts::ReadPerfEvents;
-    return 0;
-  }
-
   std::string Error;
   outs() << "PERF2BOLT: waiting for perf " << Name
          << " collection to finish...\n";
@@ -2070,6 +2056,15 @@ std::error_code DataAggregator::parseMMapEvents() {
     if (FileMMapInfo.first == "(deleted)")
       continue;
 
+    // Consider only the first mapping of the file for any given PID
+    auto Range = GlobalMMapInfo.equal_range(FileMMapInfo.first);
+    bool PIDExists = llvm::any_of(make_range(Range), [&](const auto &MI) {
+      return MI.second.PID == FileMMapInfo.second.PID;
+    });
+
+    if (PIDExists)
+      continue;
+
     GlobalMMapInfo.insert(FileMMapInfo);
   }
 
@@ -2121,22 +2116,12 @@ std::error_code DataAggregator::parseMMapEvents() {
                << " using file offset 0x" << Twine::utohexstr(MMapInfo.Offset)
                << ". Ignoring profile data for this mapping\n";
         continue;
+      } else {
+        MMapInfo.BaseAddress = *BaseAddress;
       }
-      MMapInfo.BaseAddress = *BaseAddress;
     }
 
-    // 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 mapping size.
-    const uint64_t EndAddress = MMapInfo.MMapAddress + MMapInfo.Size;
-    const uint64_t Size = EndAddress - BinaryMMapInfo[MMapInfo.PID].BaseAddress;
-    if (Size > BinaryMMapInfo[MMapInfo.PID].Size)
-      BinaryMMapInfo[MMapInfo.PID].Size = Size;
+    BinaryMMapInfo.insert(std::make_pair(MMapInfo.PID, MMapInfo));
   }
 
   if (BinaryMMapInfo.empty()) {

bolt/lib/Rewrite/RewriteInstance.cpp

@@ -2927,6 +2927,23 @@ void RewriteInstance::handleRelocation(const SectionRef &RelocatedSection,
     LLVM_DEBUG(dbgs() << "BOLT-DEBUG: ignoring relocation from data to data\n");
 }
 
+static BinaryFunction *getInitFunctionIfStaticBinary(BinaryContext &BC) {
+  // Workaround for https://github.com/llvm/llvm-project/issues/100096
+  // ("[BOLT] GOT array pointer incorrectly rewritten"). In aarch64
+  // static glibc binaries, the .init section's _init function pointer can
+  // alias with a data pointer for the end of an array. GOT rewriting
+  // currently can't detect this and updates the data pointer to the
+  // moved _init, causing a runtime crash. Skipping _init on the other
+  // hand should be harmless.
+  if (!BC.IsStaticExecutable)
+    return nullptr;
+  const BinaryData *BD = BC.getBinaryDataByName("_init");
+  if (!BD || BD->getSectionName() != ".init")
+    return nullptr;
+  LLVM_DEBUG(dbgs() << "BOLT-DEBUG: skip _init in for GOT workaround.\n");
+  return BC.getBinaryFunctionAtAddress(BD->getAddress());
+}
+
 void RewriteInstance::selectFunctionsToProcess() {
   // Extend the list of functions to process or skip from a file.
   auto populateFunctionNames = [](cl::opt<std::string> &FunctionNamesFile,
@@ -3047,6 +3064,9 @@ void RewriteInstance::selectFunctionsToProcess() {
     return true;
   };
 
+  if (BinaryFunction *Init = getInitFunctionIfStaticBinary(*BC))
+    Init->setIgnored();
+
   for (auto &BFI : BC->getBinaryFunctions()) {
     BinaryFunction &Function = BFI.second;
 

bolt/test/AArch64/check-init-not-moved.s

@@ -0,0 +1,43 @@
+# Regression test for https://github.com/llvm/llvm-project/issues/100096
+# static glibc binaries crash on startup because _init is moved and
+# shares its address with an array end pointer. The GOT rewriting can't
+# tell the two pointers apart and incorrectly updates the _array_end
+# address. Test checks that _init is not moved.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static -Wl,--section-start=.data=0x1000 -Wl,--section-start=.init=0x1004
+# RUN: llvm-bolt %t.exe -o %t.bolt
+# RUN: llvm-nm %t.exe | FileCheck --check-prefix=CHECK-ORIGINAL %s
+# RUN: llvm-nm %t.bolt | FileCheck --check-prefix=CHECK-BOLTED %s
+
+.section .data
+.globl _array_end
+_array_start:
+    .word 0x0
+
+_array_end:
+.section .init,"ax",@progbits
+.globl _init
+
+# Check that bolt doesn't move _init.
+#
+# CHECK-ORIGINAL: 0000000000001004 T _init
+# CHECK-BOLTED:   0000000000001004 T _init
+_init:
+    ret
+
+.section .text,"ax",@progbits
+.globl _start
+
+# Check that bolt is moving some other functions.
+#
+# CHECK-ORIGINAL:   0000000000001008 T _start
+# CHECK-BOLTED-NOT: 0000000000001008 T _start
+_start:
+    bl _init
+    adrp x0, #:got:_array_end
+    ldr x0, [x0, #:gotpage_lo15:_array_end]
+    adrp x0, #:got:_init
+    ldr x0, [x0, #:gotpage_lo15:_init]
+    ret
+