[BOLT] Check entry point address is not in constant island

[Asher8118]

There are cases where addEntryPointAtOffset is called with a given Offset that points to an address within a constant island. This triggers assert(!isInConstantIsland(EntryPointAddress) and causes BOLT to crash. This patch adds a check which ignores functions that would add such entry points and warns the user.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Asher Dobrescu (Asher8118)

Changes

There are cases where addEntryPointAtOffset is called with a given Offset that points to an address within a constant island. This triggers assert(!isInConstantIsland(EntryPointAddress) and causes BOLT to crash. This patch adds a check which ignores functions that would add such entry points and warns the user.

---

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

2 Files Affected:

• (modified) bolt/lib/Core/BinaryContext.cpp (+11-2)
• (added) bolt/test/AArch64/constant-island-entry.s (+35)

diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index dd0d041692484..c35775464751b 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -1336,8 +1336,17 @@ void BinaryContext::processInterproceduralReferences() {
             << Function.getPrintName() << " and "
             << TargetFunction->getPrintName() << '\n';
       }
-      if (uint64_t Offset = Address - TargetFunction->getAddress())
-        TargetFunction->addEntryPointAtOffset(Offset);
+      if (uint64_t Offset = Address - TargetFunction->getAddress()) {
+        if (!TargetFunction->isInConstantIsland(Address)) {
+          TargetFunction->addEntryPointAtOffset(Offset);
+        } else {
+          TargetFunction->setIgnored();
+          this->outs() << "BOLT-WARNING: Ignoring entry point at address 0x"
+                       << Twine::utohexstr(Address)
+                       << " in constant island of function " << *TargetFunction
+                       << '\n';
+        }
+      }
 
       continue;
     }
diff --git a/bolt/test/AArch64/constant-island-entry.s b/bolt/test/AArch64/constant-island-entry.s
new file mode 100644
index 0000000000000..bd0906d87f15f
--- /dev/null
+++ b/bolt/test/AArch64/constant-island-entry.s
@@ -0,0 +1,35 @@
+// This test checks that we ignore functions which add an entry point that
+// is in a costant island.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-linux-gnu %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
+
+# CHECK: BOLT-WARNING: Ignoring entry point at address 0x{{[0-9a-f]+}} in constant island of function func
+
+.globl func
+.type func, %function
+func:
+  ret
+  nop
+  b .Lafter_constant
+
+.type constant_island, %object
+constant_island:
+  .xword 0xabcdef
+
+.Lafter_constant:
+  ret
+  .size func, .-func
+
+.globl caller
+.type caller, %function
+caller:
+  bl constant_island
+  ret
+
+.globl main
+.type main, %function
+main:
+  bl caller
+  ret

[bgergely0]

LGTM, with a nit and a question. Let's wait for others though.

[paschalis-mpeis]

Hey Ash,

Thanks for the patch! IIUC, your use case is an entrypoint to a code island, which was marked as data?

Given we branch to it, I'm not sure if we are confident enough to use this as a heuristic, treating such cases as code and processing them fully, rather than ignoring the functions? If input code is indeed problematic, it'll crash regardless of being ignored or not.

[Asher8118]

Given we branch to it, I'm not sure if we are confident enough to use this as a heuristic, treating such cases as code and processing them fully, rather than ignoring the functions?

I could look into this in the future, however for this case it's not straightforward to check if we can process the area marked as data rather than ignore the function.

If input code is indeed problematic, it'll crash regardless of being ignored or not.

This should not be the case, if we are dealing with code marked as data then the input binary itself is not problematic, it's only BOLT that struggles to parse it.
Additionally, the functions we ignore in this patch would lead to an assertion failure anyway. So I am quite confident that we wouldn't be missing functions that could otherwise be optimised.

[paschalis-mpeis]

Thanks Ash! Yes, I was curious about an easy win; probably not as easy as #160161. But not a blocker for this patch, since BOLT would assert/crash anyway.

If input code is indeed problematic, it'll crash regardless of being ignored or not.

This should not be the case, if we are dealing with code marked as data then the input binary itself is not problematic, it's only BOLT that struggles to parse it.

To clarify: yes, that matches your use case.
I was referring to a truly problematic input binary though, ie an island not meant to be code or containing a bad instruction sequence.

In such cases, whether we ignore it and proceed (as in this patch), or use the branching as a heuristic, convert the marker and process it, it would crash anyway, but that wouldn't be BOLT's responsibility.

---

Looks good! Give it a day or two in case there are any further comments.