[BOLT] Do not reject irreversible branches during disassembly

[urnathan]

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.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Nathan Sidwell (urnathan)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/95572.diff

1 Files Affected:

• (modified) bolt/lib/Core/BinaryFunction.cpp (-7)

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.

[maksfb]

The change is in the right direction. However, we need to cover the handling of jrcxz in one of the passes. I will publish a PR with a fix to bolt/test/X86/bug-reorder-bb-jrcxz.s.

[maksfb]

#95861

[urnathan]

Thanks, that test fails with this original change (as expected). Here's an update adding isUnsupportedInstruction, and hooking that into the disassembler's main loop -- not just inside the branch/call handling, so it can trigger on any instruction. It preserves the behaviour of marking the target function of an unsupported branch as ignored too. I'm not sure if that's necessary, what was the intent there? Do X86 executables have inter-function loop and j.rcx instructions?

The X86_64 target marks the loop and j*rcx instructions as unsupported. This is the only overrider.

AFAICT, x86's isDynamicBranch check belongs in isReversibleBranch as the necessary annotation hasn't happened at the point the disassembler calls isUnsupportedInstruction. But, that check doesn't seem x86-specific, other arches could have self-modifying code, right?[*] So I moved that code to MCPlusBuilder's default isReversibleBranch implementation. I also added asserts there that we never see a non-conditional branch and never an unsupported instruction.

The Instrumentation pass change is a result of that assert, it doesn't seem to me that a special check is needed there.

The other change is in the X86 target code, and again I don't think it's needed. We just reject indirect branches at that point -- but the placement of the comment 'Handle conditional branches and ignore indirect branches' confuses me. I /think/ it's talking about the entire remainder of the loop, not just the if statment it's attached to.

The remaining call if isReversibleBranch in BinaryFunction::fixBranches appears to be a legitimate use.

[*] back in the pentium microarchitecture days, Intel had to figure out how close ahead to the PC existing self-modifying code wrote, btw

[maksfb]

Thanks, that test fails with this original change (as expected). Here's an update adding isUnsupportedInstruction, and hooking that into the disassembler's main loop -- not just inside the branch/call handling, so it can trigger on any instruction. It preserves the behaviour of marking the target function of an unsupported branch as ignored too. I'm not sure if that's necessary, what was the intent there? Do X86 executables have inter-function loop and j.rcx instructions?

We've seen inter-proc jrcxz inside gcc (cc1plus) binary. The code originated from gmp library (__gmpn_add_nc to __gmpn_add_n).

The X86_64 target marks the loop and j*rcx instructions as unsupported. This is the only overrider.

AFAICT, x86's isDynamicBranch check belongs in isReversibleBranch as the necessary annotation hasn't happened at the point the disassembler calls isUnsupportedInstruction. But, that check doesn't seem x86-specific, other arches could have self-modifying code, right?[*] So I moved that code to MCPlusBuilder's default isReversibleBranch implementation. I also added asserts there that we never see a non-conditional branch and never an unsupported instruction.

The Instrumentation pass change is a result of that assert, it doesn't seem to me that a special check is needed there.

Makes sense.

The other change is in the X86 target code, and again I don't think it's needed. We just reject indirect branches at that point -- but the placement of the comment 'Handle conditional branches and ignore indirect branches' confuses me. I /think/ it's talking about the entire remainder of the loop, not just the if statment it's attached to.

I agree. The comment is most likely outdated.

The remaining call if isReversibleBranch in BinaryFunction::fixBranches appears to be a legitimate use.

[*] back in the pentium microarchitecture days, Intel had to figure out how close ahead to the PC existing self-modifying code wrote, btw

I cannot test this change at the moment. Do you expect it to be NFC? As a follow-up, we need to add a pass that marks functions as non-optimizible/simple and move the unsupported check in there.

[urnathan]

Yes, I expect this to be NFC.

[maksfb]

Cool. Please update Title+Summary before committing. Thanks!