diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp index 80b15d7b7961..d5bfbf1e9f59 100644 --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -579,6 +579,21 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address, TrimmedSize = EntriesAsAddress->size(); }; + auto printEntryDiagnostics = [&](raw_ostream &OS, + const BinaryFunction *TargetBF) { + OS << "FAIL: function doesn't contain this address\n"; + if (!TargetBF) + return; + OS << " ! function containing this address: " << *TargetBF << '\n'; + if (!TargetBF->isFragment()) + return; + OS << " ! is a fragment with parents: "; + ListSeparator LS; + for (BinaryFunction *Parent : TargetBF->ParentFragments) + OS << LS << *Parent; + OS << '\n'; + }; + ErrorOr Section = getSectionForAddress(Address); if (!Section) return false; @@ -646,25 +661,8 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address, // Function or one of its fragments. const BinaryFunction *TargetBF = getBinaryFunctionContainingAddress(Value); - const bool DoesBelongToFunction = - BF.containsAddress(Value) || - (TargetBF && areRelatedFragments(TargetBF, &BF)); - if (!DoesBelongToFunction) { - LLVM_DEBUG({ - if (!BF.containsAddress(Value)) { - dbgs() << "FAIL: function doesn't contain this address\n"; - if (TargetBF) { - dbgs() << " ! function containing this address: " - << TargetBF->getPrintName() << '\n'; - if (TargetBF->isFragment()) { - dbgs() << " ! is a fragment"; - for (BinaryFunction *Parent : TargetBF->ParentFragments) - dbgs() << ", parent: " << Parent->getPrintName(); - dbgs() << '\n'; - } - } - } - }); + if (!TargetBF || !areRelatedFragments(TargetBF, &BF)) { + LLVM_DEBUG(printEntryDiagnostics(dbgs(), TargetBF)); break; } @@ -703,10 +701,7 @@ void BinaryContext::populateJumpTables() { ++JTI) { JumpTable *JT = JTI->second; - bool NonSimpleParent = false; - for (BinaryFunction *BF : JT->Parents) - NonSimpleParent |= !BF->isSimple(); - if (NonSimpleParent) + if (!llvm::all_of(JT->Parents, std::mem_fn(&BinaryFunction::isSimple))) continue; uint64_t NextJTAddress = 0; @@ -840,33 +835,25 @@ BinaryContext::getOrCreateJumpTable(BinaryFunction &Function, uint64_t Address, assert(JT->Type == Type && "jump table types have to match"); assert(Address == JT->getAddress() && "unexpected non-empty jump table"); - // Prevent associating a jump table to a specific fragment twice. - if (!llvm::is_contained(JT->Parents, &Function)) { - assert(llvm::all_of(JT->Parents, - [&](const BinaryFunction *BF) { - return areRelatedFragments(&Function, BF); - }) && - "cannot re-use jump table of a different function"); - // Duplicate the entry for the parent function for easy access - JT->Parents.push_back(&Function); - if (opts::Verbosity > 2) { - this->outs() << "BOLT-INFO: Multiple fragments access same jump table: " - << JT->Parents[0]->getPrintName() << "; " - << Function.getPrintName() << "\n"; - JT->print(this->outs()); - } - Function.JumpTables.emplace(Address, JT); - for (BinaryFunction *Parent : JT->Parents) - Parent->setHasIndirectTargetToSplitFragment(true); - } + if (llvm::is_contained(JT->Parents, &Function)) + return JT->getFirstLabel(); - bool IsJumpTableParent = false; - (void)IsJumpTableParent; - for (BinaryFunction *Frag : JT->Parents) - if (Frag == &Function) - IsJumpTableParent = true; - assert(IsJumpTableParent && + // Prevent associating a jump table to a specific fragment twice. + auto isSibling = std::bind(&BinaryContext::areRelatedFragments, this, + &Function, std::placeholders::_1); + assert(llvm::all_of(JT->Parents, isSibling) && "cannot re-use jump table of a different function"); + if (opts::Verbosity > 2) { + this->outs() << "BOLT-INFO: multiple fragments access the same jump table" + << ": " << *JT->Parents[0] << "; " << Function << '\n'; + JT->print(this->outs()); + } + if (JT->Parents.size() == 1) + JT->Parents.front()->setHasIndirectTargetToSplitFragment(true); + Function.setHasIndirectTargetToSplitFragment(true); + // Duplicate the entry for the parent function for easy access + JT->Parents.push_back(&Function); + Function.JumpTables.emplace(Address, JT); return JT->getFirstLabel(); } diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp index 1aad25242712..d3ed9e8088bf 100644 --- a/bolt/lib/Core/BinaryEmitter.cpp +++ b/bolt/lib/Core/BinaryEmitter.cpp @@ -809,52 +809,50 @@ void BinaryEmitter::emitJumpTable(const JumpTable &JT, MCSection *HotSection, Streamer.switchSection(JT.Count > 0 ? HotSection : ColdSection); Streamer.emitValueToAlignment(Align(JT.EntrySize)); } - MCSymbol *LastLabel = nullptr; + MCSymbol *JTLabel = nullptr; uint64_t Offset = 0; for (MCSymbol *Entry : JT.Entries) { auto LI = JT.Labels.find(Offset); - if (LI != JT.Labels.end()) { - LLVM_DEBUG({ - dbgs() << "BOLT-DEBUG: emitting jump table " << LI->second->getName() - << " (originally was at address 0x" - << Twine::utohexstr(JT.getAddress() + Offset) - << (Offset ? ") as part of larger jump table\n" : ")\n"); - }); - if (!LabelCounts.empty()) { - LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: " - << LabelCounts[LI->second] << '\n'); - if (LabelCounts[LI->second] > 0) - Streamer.switchSection(HotSection); - else - Streamer.switchSection(ColdSection); - Streamer.emitValueToAlignment(Align(JT.EntrySize)); - } - // Emit all labels registered at the address of this jump table - // to sync with our global symbol table. We may have two labels - // registered at this address if one label was created via - // getOrCreateGlobalSymbol() (e.g. LEA instructions referencing - // this location) and another via getOrCreateJumpTable(). This - // creates a race where the symbols created by these two - // functions may or may not be the same, but they are both - // registered in our symbol table at the same address. By - // emitting them all here we make sure there is no ambiguity - // that depends on the order that these symbols were created, so - // whenever this address is referenced in the binary, it is - // certain to point to the jump table identified at this - // address. - if (BinaryData *BD = BC.getBinaryDataByName(LI->second->getName())) { - for (MCSymbol *S : BD->getSymbols()) - Streamer.emitLabel(S); - } else { - Streamer.emitLabel(LI->second); - } - LastLabel = LI->second; + if (LI == JT.Labels.end()) + goto emitEntry; + JTLabel = LI->second; + LLVM_DEBUG({ + dbgs() << "BOLT-DEBUG: emitting jump table " << JTLabel->getName() + << " (originally was at address 0x" + << Twine::utohexstr(JT.getAddress() + Offset) + << (Offset ? ") as part of larger jump table\n" : ")\n"); + }); + if (!LabelCounts.empty()) { + const uint64_t JTCount = LabelCounts[JTLabel]; + LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: " << JTCount << '\n'); + Streamer.switchSection(JTCount ? HotSection : ColdSection); + Streamer.emitValueToAlignment(Align(JT.EntrySize)); } + // Emit all labels registered at the address of this jump table + // to sync with our global symbol table. We may have two labels + // registered at this address if one label was created via + // getOrCreateGlobalSymbol() (e.g. LEA instructions referencing + // this location) and another via getOrCreateJumpTable(). This + // creates a race where the symbols created by these two + // functions may or may not be the same, but they are both + // registered in our symbol table at the same address. By + // emitting them all here we make sure there is no ambiguity + // that depends on the order that these symbols were created, so + // whenever this address is referenced in the binary, it is + // certain to point to the jump table identified at this + // address. + if (BinaryData *BD = BC.getBinaryDataByName(JTLabel->getName())) { + for (MCSymbol *S : BD->getSymbols()) + Streamer.emitLabel(S); + } else { + Streamer.emitLabel(JTLabel); + } + emitEntry: if (JT.Type == JumpTable::JTT_NORMAL) { Streamer.emitSymbolValue(Entry, JT.OutputEntrySize); } else { // JTT_PIC const MCSymbolRefExpr *JTExpr = - MCSymbolRefExpr::create(LastLabel, Streamer.getContext()); + MCSymbolRefExpr::create(JTLabel, Streamer.getContext()); const MCSymbolRefExpr *E = MCSymbolRefExpr::create(Entry, Streamer.getContext()); const MCBinaryExpr *Value = diff --git a/bolt/test/X86/split-func-jump-table-fragment-bidirection.s b/bolt/test/X86/split-func-jump-table-fragment-bidirection.s index 52c816ccd900..3d3247fc58f2 100644 --- a/bolt/test/X86/split-func-jump-table-fragment-bidirection.s +++ b/bolt/test/X86/split-func-jump-table-fragment-bidirection.s @@ -10,7 +10,7 @@ # RUN: %clang %cflags %t.o -o %t.exe -Wl,-q # RUN: llvm-bolt -print-cfg -v=3 %t.exe -o %t.out 2>&1 | FileCheck %s -# CHECK: BOLT-INFO: Multiple fragments access same jump table: main; main.cold.1 +# CHECK: BOLT-INFO: multiple fragments access the same jump table: main; main.cold.1 # CHECK: PIC Jump table JUMP_TABLE1 for function main, main.cold.1 at {{.*}} with a total count of 0: .text