refactor external branch and simplify tests

Author: Jinjie-Huang

bolt/include/bolt/Core/BinaryFunction.h

@@ -2320,11 +2320,6 @@ class BinaryFunction {
   /// zero-value bytes.
   bool isZeroPaddingAt(uint64_t Offset) const;
 
-  /// Validate if the target of an external direct branch/call is a valid
-  /// executable instruction.
-  /// Return true if the target is valid, false otherwise.
-  bool validateExternalBranch(uint64_t TargetAddress);
-
   /// Validate if the target of any internal direct branch/call is a valid
   /// executable instruction.
   /// Return true if all the targets are valid, false otherwise.

bolt/lib/Core/BinaryContext.cpp

@@ -1431,16 +1431,27 @@ void BinaryContext::processInterproceduralReferences() {
             << TargetFunction->getPrintName() << '\n';
       }
 
+      const uint64_t TargetOffset = Address - TargetFunction->getAddress();
+      if (TargetOffset && (TargetOffset <= TargetFunction->getSize())) {
+        if (TargetFunction->CurrentState ==
+                BinaryFunction::State::Disassembled &&
+            (!TargetFunction->getInstructionAtOffset(TargetOffset) ||
+             TargetFunction->getSizeOfDataInCodeAt(TargetOffset)))
+          this->errs()
+              << "BOLT-WARNING: corrupted control flow detected in function "
+              << Function
+              << ", an external branch/call targets an invalid instruction "
+              << "at address 0x" << Twine::utohexstr(Address) << "\n";
+        Function.setIgnored();
+      }
+
       // Create an extra entry point if needed. Can also render the target
       // function ignored if the reference is invalid.
       handleExternalBranchTarget(Address, *TargetFunction);
 
       continue;
     }
 
-    if (!Function.validateExternalBranch(Address))
-      continue;
-
     // Check if address falls in function padding space - this could be
     // unmarked data in code. In this case adjust the padding space size.
     ErrorOr<BinarySection &> Section = getSectionForAddress(Address);

bolt/lib/Core/BinaryFunction.cpp

@@ -1910,45 +1910,8 @@ bool BinaryFunction::scanExternalRefs() {
   return Success;
 }
 
-bool BinaryFunction::validateExternalBranch(uint64_t TargetAddress) {
-  if (!isSimple())
-    return true;
-
-  BinaryFunction *TargetFunction =
-      BC.getBinaryFunctionContainingAddress(TargetAddress);
-
-  bool IsValid = true;
-
-  if (TargetFunction) {
-    const uint64_t TargetOffset = TargetAddress - TargetFunction->getAddress();
-    // Skip empty functions and out-of-bounds offsets,
-    // as they may not be disassembled.
-    if (!TargetOffset || (TargetOffset > TargetFunction->getSize()))
-      return true;
-
-    if (TargetFunction->CurrentState == State::Disassembled &&
-        (!TargetFunction->getInstructionAtOffset(TargetOffset) ||
-         getSizeOfDataInCodeAt(TargetOffset)))
-      IsValid = false;
-  } else {
-    if (!BC.getSectionForAddress(TargetAddress))
-      IsValid = false;
-  }
-
-  if (!IsValid) {
-    setIgnored();
-    BC.errs() << "BOLT-WARNING: corrupted control flow detected in function "
-              << *this
-              << ", an external branch/call targets an invalid instruction "
-              << "at address 0x" << Twine::utohexstr(TargetAddress) << "\n";
-    return false;
-  }
-
-  return true;
-}
-
 bool BinaryFunction::validateInternalBranches() {
-  if (!isSimple())
+  if (!isSimple() || TrapsOnEntry)
     return true;
 
   for (const auto &KV : Labels) {

bolt/test/AArch64/validate-branch-target.s

@@ -14,20 +14,23 @@
 .globl  internal_corrupt
 .type   internal_corrupt,@function
 internal_corrupt:
-    ret
-    nop
-.Lfake_branch_1:
-    .inst 0x14000001  // Opcode 0x14=b, check for internal branch: b + 0x4
-.Lgarbage_1:
+    b constant_island_0  // targeting the data in code
+constant_island_0:
     .word 0xffffffff
 .size   internal_corrupt,.-internal_corrupt
 
 
 .globl  external_corrupt
 .type   external_corrupt,@function
 external_corrupt:
-    ret
-    nop
-.Lfake_branch_2:
-    .inst   0x14000004  // Opcode 0x14=b, check for external branch: b + 0xf
+    b   constant_island_1  // targeting the data in code externally
 .size   external_corrupt,.-external_corrupt
+
+.globl  external_func
+.type   external_func,@function
+external_func:
+    add x0, x0, x1
+constant_island_1:
+    .word 0xffffffff // data in code
+    ret
+.size   external_func,.-external_func

bolt/test/X86/validate-branch-target.s

@@ -13,35 +13,21 @@
 
 .globl	internal_corrupt
 .type	internal_corrupt,@function
-.align	16
 internal_corrupt:
-	leaq	.Lopts_1(%rip),%rax
-	addq	$25,%rax
-	.byte	0xf3,0xc3
-.L8xchar_1:
-	addq	$12,%rax
-.Ldone_1:
-	.byte	0xf3,0xc3
-.align	64
-.Lopts_1:
-.byte	114,1,52,40,56,120,44,105,110,116,41,0  # data '114' will be disassembled as 'jb', check for internal branch: jb + 0x1
-.align	64
+	jb  data_in_code + 1  # targeting the data in code, and jump into the middle of 'xorb' instruction
+data_in_code:
+	.byte 0x34, 0x01 # data in code, will be disassembled as 'xorb 0x1, %al'
 .size	internal_corrupt,.-internal_corrupt
 
 
 .globl	external_corrupt
 .type	external_corrupt,@function
-.align	16
 external_corrupt:
-	leaq	.Lopts_2(%rip),%rax
-	addq	$25,%rax
-	.byte	0xf3,0xc3
-.L8xchar_2:
-	addq	$12,%rax
-.Ldone_2:
-	.byte	0xf3,0xc3
-.align	64
-.Lopts_2:
-.byte	114,99,52,40,56,120,44,99,104,97,114,41,0  # data '114' will be disassembled as 'jb', check for external branch: jb + 0x63
-.align	64
+	jb  external_func + 1  # targeting the middle of normal instruction externally
 .size	external_corrupt,.-external_corrupt
+
+.globl	external_func
+.type	external_func,@function
+external_func:
+	addq  $1, %rax  # normal instruction
+.size	external_func,.-external_func