[HWASan] [GlobalISel] Add +tagged-globals backend feature for GlobalISel

[hctim]

Summary

GlobalISel is the default ISel for aarch64 at -O0. Prior to D78465, GlobalISel
didn't have support for dealing with address-of-global lowerings, so it fell
back to SelectionDAGISel.

HWASan Globals require special handling, as they contain the pointer tag in the
top 16-bits, and are thus outside the code model. We need to generate a movk
in the instruction sequence with a G3 relocation to ensure the bits are
relocated properly. This is implemented in SelectionDAGISel, this patch does
the same for GlobalISel.

GlobalISel and SelectionDAGISel differ in their lowering sequence, so there are
differences in the final instruction sequence, explained in
tagged-globals.ll. Both of these implementations are correct, but GlobalISel
is slightly larger code size / slightly slower (by a couple of arithmetic
instructions). I don't see this as a problem for now as GlobalISel is only on
by default at -O0.

[hctim]

First patchset was the wrong diff. Should be the right one now.

[arsenm]

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

693

Isn't this a copy of the original operand, so the further modifications don't do anything? Needs a reference?

694

Assuming this was modifying the original instruction, it needs to notify the change observer. Also you may need to guard against already legalized globals?

[hctim]

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

693

I'm admittedly not very familiar with the backend, the goal here is to emit a *new* movk instruction w/ an immediate that's relocated with R_AARCH64_MOVW_PREL_G3.

I assume that we want to copy the operand so that we can generate the right relocation here?

694

w.r.t the change observer, we're looking to add an instruction, not modify the existing. I don't fully understand when the CO needs to be notified, as the other two created instructions in this function (adrp + addlow) don't notify the CO. Can you please help me understand when/whether this is necessary?

[hctim]

• Reformat exported-tagged-globalc.

[arsenm]

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

693

You're copying the operand, and then you don't do anything with it after modifying it; you don't add Tag to the new instruction. In any case, having a freestanding MachineOperand value is a bad idea.

You're also creating a new generic virtual register for the selected instruction, so it needs a register class. You should create one with the final register class (I'm guessing AArch64::GPR64RegClass), and then set the type with MRI.setType (the above code does the same thing in the opposite order, by creating the vreg with a type and setting the class later)

[hctim]

• Remove vestigal operand and set regclass properly.

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

693

Ah, yes - it's vestigal and can be removed, thanks!

I've also classed the vreg, thanks.

[aemerson]

I'm not familiar with these types globals. Do you think there's any issue with the current design that would make it difficult to generate optimal code in future?

[hctim]

In D82615#2117979, @aemerson wrote:

I'm not familiar with these types globals. Do you think there's any issue with the current design that would make it difficult to generate optimal code in future?

No. This codegen should only apply with HWASan (and maybe MTE) in the future -- and the only thing that could need improvement is a slight change to the codegen that would allow the lo12 add to be folded into a subsequent ldr/str. This patch doesn't do that because of the comment over in tryFoldAddLowIntoImm(..): // TODO: Need to check GV's offset % size if doing offset folding into globals.

[hctim]

Hi @arsenm - does this patch LGTY? Still unsure about whether we need to notify the CO here, if you could help advise that would be much appreciated. Thanks.

[arsenm]

LGTM. You only need the change observer if you were modifying the original instruction in place

[aemerson]

jq3ysyuS

[aemerson]

In D82615#2145444, @aemerson wrote:

jq3ysyuS

My 9 month old did this. LGTM anyway.

[hctim]

Fix an OOB operand problem. ISel is smart enough to figure out the MO_G3
relocation should use a 48-bit shift, but assert()'s internally if the third
operand isn't specified.

compiler-rt/test/hwasan/TestCases/exported-tagged-global.c

+// RUN: %clang_hwasan %s -o %t
+// RUN: %run %t
+// RUN: %clang_hwasan -O1 %s -o %t
+// RUN: %run %t
+// RUN: %clang_hwasan -O1 -mllvm --aarch64-enable-global-isel-at-O=1 %s -o %t
+// RUN: %run %t
+
+static int global;
+
+__attribute__((optnone)) int *address_of_global() { return &global; }
+
+int main(int argc, char **argv) {
+  int *global_address = address_of_global();
+  *global_address = 13;
+  return 0;
+}

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

getActionDefinitionsBuilder(G_FCMP)
       .widenScalarToNextPow2(1);
 
   // Extensions
   auto ExtLegalFunc = [=](const LegalityQuery &Query) {
     unsigned DstSize = Query.Types[0].getSizeInBits();
 
     if (DstSize == 128 && !Query.Types[0].isVector())
       return false; // Extending to a scalar s128 needs narrowing.
-    
     // Make sure that we have something that will fit in a register, and
     // make sure it's a power of 2.
     if (DstSize < 8 || DstSize > 128 || !isPowerOf2_32(DstSize))
       return false;
 
     const LLT &SrcTy = Query.Types[1];
 
     // Special case for s1.
if (OpFlags & AArch64II::MO_GOT)
     return true;
 
   Register DstReg = MI.getOperand(0).getReg();
   auto ADRP = MIRBuilder.buildInstr(AArch64::ADRP, {LLT::pointer(0, 64)}, {})
                   .addGlobalAddress(GV, 0, OpFlags | AArch64II::MO_PAGE);
   // Set the regclass on the dest reg too.
   MRI.setRegClass(ADRP.getReg(0), &AArch64::GPR64RegClass);
 
+  // MO_TAGGED on the page indicates a tagged address. Set the tag now. We do so
+  // by creating a MOVK that sets bits 48-63 of the register to (global address
+  // + 0x100000000 - PC) >> 48. The additional 0x100000000 offset here is to
+  // prevent an incorrect tag being generated during relocation when the the
+  // global appears before the code section. Without the offset, a global at
+  // `0x0f00'0000'0000'1000` (i.e. at `0x1000` with tag `0xf`) that's referenced
+  // by code at `0x2000` would result in `0x0f00'0000'0000'1000 - 0x2000 =
+  // 0x0eff'ffff'ffff'f000`, meaning the tag would be incorrectly set to `0xe`
+  // instead of `0xf`.
+  // This assumes that we're in the small code model so we can assume a binary
+  // size of <= 4GB, which makes the untagged PC relative offset positive. The
+  // binary must also be loaded into address range [0, 2^48). Both of these
+  // properties need to be ensured at runtime when using tagged addresses.
+  if (OpFlags & AArch64II::MO_TAGGED) {
+    ADRP = MIRBuilder.buildInstr(AArch64::MOVKXi, {LLT::pointer(0, 64)}, {ADRP})
+               .addGlobalAddress(GV, 0x100000000,
+                                 AArch64II::MO_PREL | AArch64II::MO_G3);
+    MRI.setRegClass(ADRP.getReg(0), &AArch64::GPR64RegClass);
+  }
+
   MIRBuilder.buildInstr(AArch64::G_ADD_LOW, {DstReg}, {ADRP})
       .addGlobalAddress(GV, 0,
                         OpFlags | AArch64II::MO_PAGEOFF | AArch64II::MO_NC);
   MI.eraseFromParent();
   return true;
 }
 
 bool AArch64LegalizerInfo::legalizeIntrinsic(

llvm/test/CodeGen/AArch64/tagged-globals.ll

-; RUN: llc < %s | FileCheck %s
+; RUN: llc --relocation-model=static < %s \
+; RUN:   | FileCheck %s --check-prefixes=CHECK-STATIC,CHECK-SELECTIONDAGISEL
+; RUN: llc --relocation-model=pic < %s \
+; RUN:   | FileCheck %s --check-prefix=CHECK-PIC
+
+; Ensure that GlobalISel lowers correctly. GlobalISel is the default ISel for
+; -O0 on aarch64. GlobalISel lowers the instruction sequence in the static
+; relocation model different to SelectionDAGISel. GlobalISel does the lowering
+; of AddLow *after* legalization, and thus doesn't differentiate between
+; address-taken-only vs. address-taken-for-loadstore. Hence, we generate a movk
+; instruction for load/store instructions as well with GlobalISel. GlobalISel
+; also doesn't have the scaffolding to correctly check the bounds of the global
+; offset, and cannot fold the lo12 bits into the load/store. Neither of these
+; things are a problem as GlobalISel is only used by default at -O0, so we don't
+; mind the code size and performance increase.
+
+; RUN: llc --aarch64-enable-global-isel-at-O=0 -O0 < %s \
+; RUN:   | FileCheck %s --check-prefixes=CHECK-STATIC,CHECK-GLOBALISEL
+; RUN: llc --aarch64-enable-global-isel-at-O=0 -O0 --relocation-model=pic < %s \
+; RUN:   | FileCheck %s --check-prefix=CHECK-PIC
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64-unknown-linux-android"
 
-@global = external hidden global i32
+@global = external global i32
 declare void @func()
 
 define i32* @global_addr() #0 {
-  ; CHECK: global_addr:
-  ; CHECK: adrp x0, :pg_hi21_nc:global
-  ; CHECK: movk x0, #:prel_g3:global+4294967296
-  ; CHECK: add x0, x0, :lo12:global
+  ; Static relocation model has common codegen between SelectionDAGISel and
+  ; GlobalISel when the address-taken of a global isn't folded into a load or
+  ; store instruction.
+  ; CHECK-STATIC: global_addr:
+  ; CHECK-STATIC: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK-STATIC: movk [[REG]], #:prel_g3:global+4294967296
+  ; CHECK-STATIC: add x0, [[REG]], :lo12:global
+  ; CHECK-STATIC: ret
+
+  ; CHECK-PIC: global_addr:
+  ; CHECK-PIC: adrp [[REG:x[0-9]+]], :got:global
+  ; CHECK-PIC: ldr x0, {{\[}}[[REG]], :got_lo12:global]
+  ; CHECK-PIC: ret
+
   ret i32* @global
 }
 
 define i32 @global_load() #0 {
-  ; CHECK: global_load:
-  ; CHECK: adrp x8, :pg_hi21_nc:global
-  ; CHECK: ldr w0, [x8, :lo12:global]
+  ; CHECK-SELECTIONDAGISEL: global_load:
+  ; CHECK-SELECTIONDAGISEL: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK-SELECTIONDAGISEL: ldr w0, {{\[}}[[REG]], :lo12:global{{\]}}
+  ; CHECK-SELECTIONDAGISEL: ret
+
+  ; CHECK-GLOBALISEL: global_load:
+  ; CHECK-GLOBALISEL: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK-GLOBALISEL: movk [[REG]], #:prel_g3:global+4294967296
+  ; CHECK-GLOBALISEL: add [[REG]], [[REG]], :lo12:global
+  ; CHECK-GLOBALISEL: ldr w0, {{\[}}[[REG]]{{\]}}
+  ; CHECK-GLOBALISEL: ret
+
+  ; CHECK-PIC: global_load:
+  ; CHECK-PIC: adrp [[REG:x[0-9]+]], :got:global
+  ; CHECK-PIC: ldr  [[REG]], {{\[}}[[REG]], :got_lo12:global]
+  ; CHECK-PIC: ldr w0, {{\[}}[[REG]]{{\]}}
+  ; CHECK-PIC: ret
+
   %load = load i32, i32* @global
   ret i32 %load
 }
 
+define void @global_store() #0 {
+  ; CHECK-SELECTIONDAGISEL: global_store:
+  ; CHECK-SELECTIONDAGISEL: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK-SELECTIONDAGISEL: str wzr, {{\[}}[[REG]], :lo12:global{{\]}}
+  ; CHECK-SELECTIONDAGISEL: ret
+
+  ; CHECK-GLOBALISEL: global_store:
+  ; CHECK-GLOBALISEL: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK-GLOBALISEL: movk [[REG]], #:prel_g3:global+4294967296
+  ; CHECK-GLOBALISEL: add [[REG]], [[REG]], :lo12:global
+  ; CHECK-GLOBALISEL: str wzr, {{\[}}[[REG]]{{\]}}
+  ; CHECK-GLOBALISEL: ret
+
+  ; CHECK-PIC: global_store:
+  ; CHECK-PIC: adrp [[REG:x[0-9]+]], :got:global
+  ; CHECK-PIC: ldr  [[REG]], {{\[}}[[REG]], :got_lo12:global]
+  ; CHECK-PIC: str wzr, {{\[}}[[REG]]{{\]}}
+  ; CHECK-PIC: ret
+
+  store i32 0, i32* @global
+  ret void
+}
+
 define void ()* @func_addr() #0 {
-  ; CHECK: func_addr:
-  ; CHECK: adrp x0, func
-  ; CHECK: add x0, x0, :lo12:func
+  ; CHECK-STATIC: func_addr:
+  ; CHECK-STATIC: adrp [[REG:x[0-9]+]], func
+  ; CHECK-STATIC: add x0, [[REG]], :lo12:func
+  ; CHECK-STATIC: ret
+
+  ; CHECK-PIC: func_addr:
+  ; CHECK-PIC: adrp [[REG:x[0-9]+]], :got:func
+  ; CHECK-PIC: ldr  x0, {{\[}}[[REG]], :got_lo12:func]
+  ; CHECK-PIC: ret
+
   ret void ()* @func
 }
 
 attributes #0 = { "target-features"="+tagged-globals" }