[PATCH] D127924: [BOLT] Resolve crash when optimizing Python

[Huan Nguyen via Phabricator via llvm-commits]

nhuhuan created this revision.
Herald added a reviewer: rafauler.
Herald added a subscriber: ayermolo.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a project: All.
nhuhuan requested review of this revision.
Herald added subscribers: llvm-commits, yota9.
Herald added a project: LLVM.

Resolve a crash related to split functions

Due to split function optimization, a function can be splitted to two
fragments, and both fragments can access same jump table. This violates
the assumption that a jump table can only have one parent function,
leading to a crash when optimizing Python.

We want to support the case: different functions cannot access same
jump tables, but different fragments of same function can!

- As all fragments are from same function, we point JT::Parent to one

specific fragment. Right now it is the first disassembled fragment, but
we can point it to the function's main fragment later.

- Functions are disassembled sequentially. Previously, at the end of

processing a function, JT::OffsetEntries is cleared, so other fragment
can no longer reuse JT::OffsetEntries. To extend the support for split
function, we only clear JT::OffsetEntries after all functions are
disassembled.

- Let say A.hot and A.cold access JT of three targets {X, Y, Z}, where

X and Y are in A.hot, and Z is in A.cold. Suppose that A.hot is
disassembled first, JT::OffsetEntries = {X',Y',INVALID_OFFSET}. When
A.cold is disassembled, it cannot reuse JT::OffsetEntries above due to
different fragment start. A simple solution:
A.hot  = {X',Y',INVALID_OFFSET}
A.cold = {INVALID_OFFSET, INVALID_OFFSET, INVALID_OFFSET}

- We update the assertion to allow different fragments of same function

to get the same JumpTable object.

The next goal:
A.hot  = {X',Y',INVALID_OFFSET}
A.cold = {INVALID_OFFSET, INVALID_OFFSET, Z'}
The main issue is A.hot and A.cold have separate CFGs, thus jump table
targets are still constrained within fragment bounds.

The stretch goal:
A.hot  = {X, Y, Z}
A.cold = {X, Y, Z}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127924

Files:
  bolt/include/bolt/Core/BinaryContext.h
  bolt/lib/Core/BinaryContext.cpp
  bolt/lib/Core/BinaryFunction.cpp


Index: bolt/lib/Core/BinaryFunction.cpp
===================================================================
--- bolt/lib/Core/BinaryFunction.cpp
+++ bolt/lib/Core/BinaryFunction.cpp
@@ -1756,12 +1756,6 @@
   }
   clearList(JTSites);
 
-  // Free memory used by jump table offsets.
-  // for (auto &JTI : JumpTables) {
-  //   JumpTable &JT = *JTI.second;
-  //   clearList(JT.OffsetEntries);
-  // }
-
   // Conservatively populate all possible destinations for unknown indirect
   // branches.
   if (opts::StrictMode && hasInternalReference()) {
Index: bolt/lib/Core/BinaryContext.cpp
===================================================================
--- bolt/lib/Core/BinaryContext.cpp
+++ bolt/lib/Core/BinaryContext.cpp
@@ -761,19 +761,21 @@
 const MCSymbol *
 BinaryContext::getOrCreateJumpTable(BinaryFunction &Function, uint64_t Address,
                                     JumpTable::JumpTableType Type) {
+  auto isFragmentOf = [](BinaryFunction *Fragment, BinaryFunction *Parent) {
+    return (Fragment->isFragment() && Fragment->isParentFragment(Parent));
+  };
+
   if (JumpTable *JT = getJumpTableContainingAddress(Address)) {
     assert(JT->Type == Type && "jump table types have to match");
-    assert((JT->Parent == &Function ||
-            (JT->Parent->isFragment() &&
-             JT->Parent->isParentFragment(&Function)) ||
-            (Function.isFragment() && Function.isParentFragment(JT->Parent))) &&
+    assert((JT->Parent == &Function || isFragmentOf(JT->Parent, &Function) ||
+            isFragmentOf(&Function, JT->Parent)) &&
            "cannot re-use jump table of a different function");
     assert(Address == JT->getAddress() && "unexpected non-empty jump table");
 
     // Flush OffsetEntries with INVALID_OFFSET if multiple parents
     // Duplicate the entry for the parent function for easy access
-    if ((JT->Parent->isFragment() && JT->Parent->isParentFragment(&Function)) ||
-        (Function.isFragment() && Function.isParentFragment(JT->Parent))) {
+    if (isFragmentOf(JT->Parent, &Function) ||
+        isFragmentOf(&Function, JT->Parent)) {
       constexpr uint64_t INVALID_OFFSET = std::numeric_limits<uint64_t>::max();
       for (unsigned I = 0; I < JT->OffsetEntries.size(); ++I)
         JT->OffsetEntries[I] = INVALID_OFFSET;
Index: bolt/include/bolt/Core/BinaryContext.h
===================================================================
--- bolt/include/bolt/Core/BinaryContext.h
+++ bolt/include/bolt/Core/BinaryContext.h
@@ -481,7 +481,7 @@
   /// to function \p BF.
   std::string generateJumpTableName(const BinaryFunction &BF, uint64_t Address);
 
-  /// Free meory used by jump table offsets
+  /// Free memory used by jump table offsets
   void clearJumpTableOffsets() {
      for (auto& JTI: JumpTables) {
         JumpTable& JT = *JTI.second;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D127924.437408.patch
Type: text/x-patch
Size: 2824 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220616/8c780e2c/attachment.bin>