Skip to content

Commit ba93fe9

Browse files
aaupovaaupov
authored
[BOLT][NFC] Simplify getOrCreate/analyze/populate/emitJumpTable (#132108)
1 parent 04c3898 commit ba93fe9

File tree

3 files changed

+72
-87
lines changed

3 files changed

+72
-87
lines changed

‎bolt/lib/Core/BinaryContext.cpp‎

Lines changed: 35 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,21 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address,
579579
TrimmedSize = EntriesAsAddress->size();
580580
};
581581

582+
auto printEntryDiagnostics = [&](raw_ostream &OS,
583+
const BinaryFunction *TargetBF) {
584+
OS << "FAIL: function doesn't contain this address\n";
585+
if (!TargetBF)
586+
return;
587+
OS << " ! function containing this address: " << *TargetBF << '\n';
588+
if (!TargetBF->isFragment())
589+
return;
590+
OS << " ! is a fragment with parents: ";
591+
ListSeparator LS;
592+
for (BinaryFunction *Parent : TargetBF->ParentFragments)
593+
OS << LS << *Parent;
594+
OS << '\n';
595+
};
596+
582597
ErrorOr<const BinarySection &> Section = getSectionForAddress(Address);
583598
if (!Section)
584599
return false;
@@ -646,25 +661,8 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address,
646661

647662
// Function or one of its fragments.
648663
const BinaryFunction *TargetBF = getBinaryFunctionContainingAddress(Value);
649-
const bool DoesBelongToFunction =
650-
BF.containsAddress(Value) ||
651-
(TargetBF && areRelatedFragments(TargetBF, &BF));
652-
if (!DoesBelongToFunction) {
653-
LLVM_DEBUG({
654-
if (!BF.containsAddress(Value)) {
655-
dbgs() << "FAIL: function doesn't contain this address\n";
656-
if (TargetBF) {
657-
dbgs() << " ! function containing this address: "
658-
<< TargetBF->getPrintName() << '\n';
659-
if (TargetBF->isFragment()) {
660-
dbgs() << " ! is a fragment";
661-
for (BinaryFunction *Parent : TargetBF->ParentFragments)
662-
dbgs() << ", parent: " << Parent->getPrintName();
663-
dbgs() << '\n';
664-
}
665-
}
666-
}
667-
});
664+
if (!TargetBF || !areRelatedFragments(TargetBF, &BF)) {
665+
LLVM_DEBUG(printEntryDiagnostics(dbgs(), TargetBF));
668666
break;
669667
}
670668

@@ -703,10 +701,7 @@ void BinaryContext::populateJumpTables() {
703701
++JTI) {
704702
JumpTable *JT = JTI->second;
705703

706-
bool NonSimpleParent = false;
707-
for (BinaryFunction *BF : JT->Parents)
708-
NonSimpleParent |= !BF->isSimple();
709-
if (NonSimpleParent)
704+
if (!llvm::all_of(JT->Parents, std::mem_fn(&BinaryFunction::isSimple)))
710705
continue;
711706

712707
uint64_t NextJTAddress = 0;
@@ -840,33 +835,25 @@ BinaryContext::getOrCreateJumpTable(BinaryFunction &Function, uint64_t Address,
840835
assert(JT->Type == Type && "jump table types have to match");
841836
assert(Address == JT->getAddress() && "unexpected non-empty jump table");
842837

843-
// Prevent associating a jump table to a specific fragment twice.
844-
if (!llvm::is_contained(JT->Parents, &Function)) {
845-
assert(llvm::all_of(JT->Parents,
846-
[&](const BinaryFunction *BF) {
847-
return areRelatedFragments(&Function, BF);
848-
}) &&
849-
"cannot re-use jump table of a different function");
850-
// Duplicate the entry for the parent function for easy access
851-
JT->Parents.push_back(&Function);
852-
if (opts::Verbosity > 2) {
853-
this->outs() << "BOLT-INFO: Multiple fragments access same jump table: "
854-
<< JT->Parents[0]->getPrintName() << "; "
855-
<< Function.getPrintName() << "\n";
856-
JT->print(this->outs());
857-
}
858-
Function.JumpTables.emplace(Address, JT);
859-
for (BinaryFunction *Parent : JT->Parents)
860-
Parent->setHasIndirectTargetToSplitFragment(true);
861-
}
838+
if (llvm::is_contained(JT->Parents, &Function))
839+
return JT->getFirstLabel();
862840

863-
bool IsJumpTableParent = false;
864-
(void)IsJumpTableParent;
865-
for (BinaryFunction *Frag : JT->Parents)
866-
if (Frag == &Function)
867-
IsJumpTableParent = true;
868-
assert(IsJumpTableParent &&
841+
// Prevent associating a jump table to a specific fragment twice.
842+
auto isSibling = std::bind(&BinaryContext::areRelatedFragments, this,
843+
&Function, std::placeholders::_1);
844+
assert(llvm::all_of(JT->Parents, isSibling) &&
869845
"cannot re-use jump table of a different function");
846+
if (opts::Verbosity > 2) {
847+
this->outs() << "BOLT-INFO: multiple fragments access the same jump table"
848+
<< ": " << *JT->Parents[0] << "; " << Function << '\n';
849+
JT->print(this->outs());
850+
}
851+
if (JT->Parents.size() == 1)
852+
JT->Parents.front()->setHasIndirectTargetToSplitFragment(true);
853+
Function.setHasIndirectTargetToSplitFragment(true);
854+
// Duplicate the entry for the parent function for easy access
855+
JT->Parents.push_back(&Function);
856+
Function.JumpTables.emplace(Address, JT);
870857
return JT->getFirstLabel();
871858
}
872859

‎bolt/lib/Core/BinaryEmitter.cpp‎

Lines changed: 36 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -809,52 +809,50 @@ void BinaryEmitter::emitJumpTable(const JumpTable &JT, MCSection *HotSection,
809809
Streamer.switchSection(JT.Count > 0 ? HotSection : ColdSection);
810810
Streamer.emitValueToAlignment(Align(JT.EntrySize));
811811
}
812-
MCSymbol *LastLabel = nullptr;
812+
MCSymbol *JTLabel = nullptr;
813813
uint64_t Offset = 0;
814814
for (MCSymbol *Entry : JT.Entries) {
815815
auto LI = JT.Labels.find(Offset);
816-
if (LI != JT.Labels.end()) {
817-
LLVM_DEBUG({
818-
dbgs() << "BOLT-DEBUG: emitting jump table " << LI->second->getName()
819-
<< " (originally was at address 0x"
820-
<< Twine::utohexstr(JT.getAddress() + Offset)
821-
<< (Offset ? ") as part of larger jump table\n" : ")\n");
822-
});
823-
if (!LabelCounts.empty()) {
824-
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: "
825-
<< LabelCounts[LI->second] << '\n');
826-
if (LabelCounts[LI->second] > 0)
827-
Streamer.switchSection(HotSection);
828-
else
829-
Streamer.switchSection(ColdSection);
830-
Streamer.emitValueToAlignment(Align(JT.EntrySize));
831-
}
832-
// Emit all labels registered at the address of this jump table
833-
// to sync with our global symbol table. We may have two labels
834-
// registered at this address if one label was created via
835-
// getOrCreateGlobalSymbol() (e.g. LEA instructions referencing
836-
// this location) and another via getOrCreateJumpTable(). This
837-
// creates a race where the symbols created by these two
838-
// functions may or may not be the same, but they are both
839-
// registered in our symbol table at the same address. By
840-
// emitting them all here we make sure there is no ambiguity
841-
// that depends on the order that these symbols were created, so
842-
// whenever this address is referenced in the binary, it is
843-
// certain to point to the jump table identified at this
844-
// address.
845-
if (BinaryData *BD = BC.getBinaryDataByName(LI->second->getName())) {
846-
for (MCSymbol *S : BD->getSymbols())
847-
Streamer.emitLabel(S);
848-
} else {
849-
Streamer.emitLabel(LI->second);
850-
}
851-
LastLabel = LI->second;
816+
if (LI == JT.Labels.end())
817+
goto emitEntry;
818+
JTLabel = LI->second;
819+
LLVM_DEBUG({
820+
dbgs() << "BOLT-DEBUG: emitting jump table " << JTLabel->getName()
821+
<< " (originally was at address 0x"
822+
<< Twine::utohexstr(JT.getAddress() + Offset)
823+
<< (Offset ? ") as part of larger jump table\n" : ")\n");
824+
});
825+
if (!LabelCounts.empty()) {
826+
const uint64_t JTCount = LabelCounts[JTLabel];
827+
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: " << JTCount << '\n');
828+
Streamer.switchSection(JTCount ? HotSection : ColdSection);
829+
Streamer.emitValueToAlignment(Align(JT.EntrySize));
830+
}
831+
// Emit all labels registered at the address of this jump table
832+
// to sync with our global symbol table. We may have two labels
833+
// registered at this address if one label was created via
834+
// getOrCreateGlobalSymbol() (e.g. LEA instructions referencing
835+
// this location) and another via getOrCreateJumpTable(). This
836+
// creates a race where the symbols created by these two
837+
// functions may or may not be the same, but they are both
838+
// registered in our symbol table at the same address. By
839+
// emitting them all here we make sure there is no ambiguity
840+
// that depends on the order that these symbols were created, so
841+
// whenever this address is referenced in the binary, it is
842+
// certain to point to the jump table identified at this
843+
// address.
844+
if (BinaryData *BD = BC.getBinaryDataByName(JTLabel->getName())) {
845+
for (MCSymbol *S : BD->getSymbols())
846+
Streamer.emitLabel(S);
847+
} else {
848+
Streamer.emitLabel(JTLabel);
852849
}
850+
emitEntry:
853851
if (JT.Type == JumpTable::JTT_NORMAL) {
854852
Streamer.emitSymbolValue(Entry, JT.OutputEntrySize);
855853
} else { // JTT_PIC
856854
const MCSymbolRefExpr *JTExpr =
857-
MCSymbolRefExpr::create(LastLabel, Streamer.getContext());
855+
MCSymbolRefExpr::create(JTLabel, Streamer.getContext());
858856
const MCSymbolRefExpr *E =
859857
MCSymbolRefExpr::create(Entry, Streamer.getContext());
860858
const MCBinaryExpr *Value =

‎bolt/test/X86/split-func-jump-table-fragment-bidirection.s‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
1111
# RUN: llvm-bolt -print-cfg -v=3 %t.exe -o %t.out 2>&1 | FileCheck %s
1212

13-
# CHECK: BOLT-INFO: Multiple fragments access same jump table: main; main.cold.1
13+
# CHECK: BOLT-INFO: multiple fragments access the same jump table: main; main.cold.1
1414
# CHECK: PIC Jump table JUMP_TABLE1 for function main, main.cold.1 at {{.*}} with a total count of 0:
1515

1616
.text

0 commit comments

Comments
 (0)