From 17f179d009560d7c6d86cc93c03e0deccc681484 Mon Sep 17 00:00:00 2001 From: Nathan Sidwell Date: Fri, 14 Jun 2024 13:00:20 -0400 Subject: [PATCH 1/2] [BOLT] Do not reject irreversible branches during disassembly Now that `isUnsupportedBranch` is renamed (and inverted) to `isReversibleBranch`, this use in `BinaryFunction::disassemble` sticks out like a sore thumb. My guess is that at one point one needed to reject unintelligible branches early, and that morphed into the use for detecting uninvertible branches. Removing this check results in no new test failures. The use of `isReversibleBranch` here raises questions about what it should return for instructions that are not conditional branches -- given the other uses of this function I had been presuming 'don't care'. If some branches should be rejected at this point, a new MCPlusBuilder hook is most likely needed -- this cannot be the correct predicate to use (after all, it would render all the other uses moot. --- bolt/lib/Core/BinaryFunction.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index d13e28999a05c..a8b1f69166d21 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -1288,13 +1288,6 @@ Error BinaryFunction::disassemble() { const bool IsCondBranch = MIB->isConditionalBranch(Instruction); MCSymbol *TargetSymbol = nullptr; - if (!BC.MIB->isReversibleBranch(Instruction)) { - setIgnored(); - if (BinaryFunction *TargetFunc = - BC.getBinaryFunctionContainingAddress(TargetAddress)) - TargetFunc->setIgnored(); - } - if (IsCall && containsAddress(TargetAddress)) { if (TargetAddress == getAddress()) { // Recursive call. From 0408a96e6a6cc7a19cdc70e298d24c572e09f1cb Mon Sep 17 00:00:00 2001 From: Nathan Sidwell Date: Fri, 21 Jun 2024 14:54:13 -0400 Subject: [PATCH 2/2] Add new isUnsupportedInst & adjust isReversibleBranch --- bolt/include/bolt/Core/MCPlusBuilder.h | 15 ++++++++++++++- bolt/lib/Core/BinaryFunction.cpp | 9 +++++++++ bolt/lib/Passes/Instrumentation.cpp | 3 +-- bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 18 ++++++++---------- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index 765372aa9e402..83f4d4c649fd8 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -439,7 +439,20 @@ class MCPlusBuilder { } /// Check whether this conditional branch can be reversed - virtual bool isReversibleBranch(const MCInst &Inst) const { return true; } + virtual bool isReversibleBranch(const MCInst &Inst) const { + assert(!isUnsupportedInstruction(Inst) && isConditionalBranch(Inst) && + "Instruction is not known conditional branch"); + + if (isDynamicBranch(Inst)) + return false; + return true; + } + + /// Return true if this instruction inhibits analysis of the containing + /// function. + virtual bool isUnsupportedInstruction(const MCInst &Inst) const { + return false; + } /// Return true of the instruction is of pseudo kind. virtual bool isPseudo(const MCInst &Inst) const { diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index a8b1f69166d21..d2ca0fcdf6b7a 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -1275,6 +1275,10 @@ Error BinaryFunction::disassemble() { } } + bool IsUnsupported = BC.MIB->isUnsupportedInstruction(Instruction); + if (IsUnsupported) + setIgnored(); + if (MIB->isBranch(Instruction) || MIB->isCall(Instruction)) { uint64_t TargetAddress = 0; if (MIB->evaluateBranch(Instruction, AbsoluteInstrAddr, Size, @@ -1288,6 +1292,11 @@ Error BinaryFunction::disassemble() { const bool IsCondBranch = MIB->isConditionalBranch(Instruction); MCSymbol *TargetSymbol = nullptr; + if (IsUnsupported) + if (auto *TargetFunc = + BC.getBinaryFunctionContainingAddress(TargetAddress)) + TargetFunc->setIgnored(); + if (IsCall && containsAddress(TargetAddress)) { if (TargetAddress == getAddress()) { // Recursive call. diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp index 14f506f9ca968..e824a42d82696 100644 --- a/bolt/lib/Passes/Instrumentation.cpp +++ b/bolt/lib/Passes/Instrumentation.cpp @@ -479,8 +479,7 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function, HasJumpTable = true; else if (BC.MIB->isUnconditionalBranch(Inst)) HasUnconditionalBranch = true; - else if ((!BC.MIB->isCall(Inst) && !BC.MIB->isConditionalBranch(Inst)) || - !BC.MIB->isReversibleBranch(Inst)) + else if (!(BC.MIB->isCall(Inst) || BC.MIB->isConditionalBranch(Inst))) continue; const uint32_t FromOffset = *BC.MIB->getOffset(Inst); diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp index 515c9a94c58cd..5bd77958934f9 100644 --- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp +++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp @@ -328,19 +328,19 @@ class X86MCPlusBuilder : public MCPlusBuilder { return false; } - bool isReversibleBranch(const MCInst &Inst) const override { - if (isDynamicBranch(Inst)) - return false; - + bool isUnsupportedInstruction(const MCInst &Inst) const override { switch (Inst.getOpcode()) { default: - return true; + return false; + case X86::LOOP: case X86::LOOPE: case X86::LOOPNE: case X86::JECXZ: case X86::JRCXZ: - return false; + // These have a short displacement, and therefore (often) break after + // basic block relayout. + return true; } } @@ -1879,11 +1879,9 @@ class X86MCPlusBuilder : public MCPlusBuilder { continue; } - // Handle conditional branches and ignore indirect branches - if (isReversibleBranch(*I) && getCondCode(*I) == X86::COND_INVALID) { - // Indirect branch + // Ignore indirect branches + if (getCondCode(*I) == X86::COND_INVALID) return false; - } if (CondBranch == nullptr) { const MCSymbol *TargetBB = getTargetSymbol(*I);