[BOLT] Add constant island check in scanExternalRefs()

[Jinjie-Huang]

The previous patch has added a check to prevent adding an entry point into a constant island, but only for successfully disassembled functions.

Because scanExternalRefs() is also called when a function fails to be disassembled or is skipped, it can still attempt to add an entry point at constant islands. The same issue may occur if without a check for it

So, this patch complements the 'constant island' check in scanExternalRefs().

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Jinjie Huang (Jinjie-Huang)

Changes

The previous patch has added a check to prevent adding an entry point into a constant island, but only for successfully disassembled functions.

Because scanExternalRefs() is also called when a function fails to be disassembled or is skipped, it can still attempt to add an entry point at constant islands. Without a check for it, the same issue occurs.

So, this patch complements the 'constant island' check in scanExternalRefs().

---

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

2 Files Affected:

• (modified) bolt/lib/Core/BinaryFunction.cpp (+13-3)
• (modified) bolt/test/AArch64/constant-island-entry.s (+1)

diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 84023efe1084e..f8ef80d75653f 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1699,9 +1699,19 @@ bool BinaryFunction::scanExternalRefs() {
 
       const uint64_t FunctionOffset =
           TargetAddress - TargetFunction->getAddress();
-      BranchTargetSymbol =
-          FunctionOffset ? TargetFunction->addEntryPointAtOffset(FunctionOffset)
-                         : TargetFunction->getSymbol();
+      if (!TargetFunction->isInConstantIsland(TargetAddress)) {
+        BranchTargetSymbol =
+            FunctionOffset ? TargetFunction->addEntryPointAtOffset(FunctionOffset)
+                          : TargetFunction->getSymbol();
+      } else {
+        TargetFunction->setIgnored();
+        Success = false;
+        BC.outs() << "BOLT-WARNING: Ignoring entry point at address 0x"
+                      << Twine::utohexstr(Address)
+                      << " in constant island of function " << *TargetFunction
+                      << '\n';
+        break;
+      }
     }
 
     // Can't find more references. Not creating relocations since we are not
diff --git a/bolt/test/AArch64/constant-island-entry.s b/bolt/test/AArch64/constant-island-entry.s
index 6567114eb980a..2bf10526c601c 100644
--- a/bolt/test/AArch64/constant-island-entry.s
+++ b/bolt/test/AArch64/constant-island-entry.s
@@ -4,6 +4,7 @@
 # RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
 # RUN: %clang %cflags %t.o -pie -Wl,-q -o %t.exe
 # RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+# RUN: llvm-bolt %t.exe -o %t.bolt -skip-funcs=caller 2>&1 | FileCheck %s
 
 # CHECK: BOLT-WARNING: Ignoring entry point at address 0x{{[0-9a-f]+}} in constant island of function func
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Asher8118]

I agree with Gergely, it would be nice to add a comment to document the new change.

Otherwise the patch looks good to me. I will leave this for one of the maintainers to give their approval to merge.

[maksfb]

Looks good - thanks! I would refactor code to eliminate code duplication by introducing something like BC.handleCodeRef(), but it's not a hard requirement and I can do it once you commit the change.

Please shorten the title before the commit.