[BOLT][AArch64] Handle external references to the middle of Constant Islands

[treapster]

Summary

Fix BinaryContext::handleAddressRef to properly detect references to other function's Constant islands.

[treapster]

Fix test, handle case when IslandIter == AddressToConstantIslandMap.end()

[treapster]

clang-format

[rafauler]

Thanks for fixing this! LGTM

[yota9]

bolt/lib/Core/BinaryContext.cpp

399

399

I assume we need to add extra check that we're still in bounds of the function. E.g. for the case when IslandIter was originally eq to AddressToConstantIslandMap.end() and we've decremented the iterator.

[treapster]

bolt/lib/Core/BinaryContext.cpp

399

getOrCreateIslandAccess (which is called by getOrCreateProxyIslandAccess) returns nullptr if !isInConstantIsland(Address))

[treapster]

bolt/lib/Core/BinaryContext.cpp

399

It should be IslandIter = std::prev(IslandIter) or std::advance(IslandIter, -1), but i think there's no need to be verbose here. Why not leave it as a decrement?

[yota9]

bolt/lib/Core/BinaryContext.cpp

399

Than it looks like "IslandIter == AddressToConstantIslandMap.begin()" check above is redundant too (it might be rewritten that iterator not equal to begin() before decrement and without else case). Although I don't might to have both your and mine checks for better code flow control, my IMHO that we need either check both edge cases here or none.

[treapster]

bolt/lib/Core/BinaryContext.cpp

399

You're probably right, i'll change to

if (IslandIter != AddressToConstantIslandMap.begin())
  --IslandIter;

[yota9]

bolt/lib/Core/BinaryContext.cpp

399

Yes, I meant that. Because of the standard https://en.cppreference.com/w/cpp/iterator/prev checkout Note section.

[treapster]

bolt/lib/Core/BinaryContext.cpp

399

I know that, bit IslandIter is not an rvalue

[yota9]

bolt/lib/Core/BinaryContext.cpp

399

You're right, for some reason I was sure that it was telling about operation on the it == end(), not the end() itself.. NMW

[treapster]

Remove redundant checks

[yota9]

bolt/lib/Core/BinaryContext.cpp

396

Unneeded parentheses

[treapster]

Remove unneeded braces

[yota9]

LGTM

bolt/lib/Core/BinaryContext.cpp

if (isAArch64()) {
     if (MCSymbol *IslandSym = BF.getOrCreateIslandAccess(Address))
       return std::make_pair(IslandSym, Addend);
 
     // Detect custom code written in assembly that refers to arbitrary
     // constant islands from other functions. Write this reference so we
     // can pull this constant island and emit it as part of this function
     // too.
     auto IslandIter = AddressToConstantIslandMap.lower_bound(Address);
+
+    if (IslandIter != AddressToConstantIslandMap.begin() &&
+        (IslandIter == AddressToConstantIslandMap.end() ||
+         IslandIter->first > Address))
+      --IslandIter;
+
     if (IslandIter != AddressToConstantIslandMap.end()) {
       if (MCSymbol *IslandSym =
               IslandIter->second->getOrCreateProxyIslandAccess(Address, BF)) {
         BF.createIslandDependency(IslandSym, IslandIter->second);
         return std::make_pair(IslandSym, Addend);
       }
     }
   }
 

bolt/test/AArch64/ext-island-ref.s

+// This test checks that references to the middle of CI from other function
+// are handled properly
+
+// RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+// RUN: %clang %cflags --target=aarch64-unknown-linux %t.o -o %t.exe -Wl,-q
+// RUN: llvm-bolt %t.exe -o %t.bolt -lite=false
+// RUN: llvm-objdump -d -j .text %t.bolt | FileCheck %s
+
+// CHECK: deadbeef
+// CHECK-NEXT: deadbeef
+// CHECK-NEXT: [[#%x,ADDR:]]: deadbeef
+// CHECK-NEXT: deadbeef
+// CHECK: <func>:
+// CHECK-NEXT: ldr  x0, 0x[[#ADDR]]
+// CHECK-NEXT: ret
+
+.type funcWithIsland, %function
+funcWithIsland:
+    ret
+    .word 0xdeadbeef
+    .word 0xdeadbeef
+    .word 0xdeadbeef
+    .word 0xdeadbeef
+.size   funcWithIsland, .-funcWithIsland
+
+.type func, %function
+func:
+    ldr x0, funcWithIsland + 12
+    ret
+.size   func, .-func
+
+.global main
+.type main, %function
+main:
+    bl funcWithIsland
+    bl func
+    mov     w8, #93
+    svc     #0
+.size   main, .-main