[HWASan] Disable GlobalISel/FastISel for HWASan Globals.

[hctim]

Summary

HWASan globals instruction selection lowering hasn't been implemented in
GlobalISel or FastISel. We need to ensure that we get SelectionDAGISel
in all cases where HWASan is enabled.

GlobalISel is the default ISel for aarch64 at -O0. All others go to
SelectionDAGISel. When we disable GlobalISel, we get FastISel, so
disable that too.

Add a regression test to HWASan, and tighten up the instruction
selection test to also include both PIC and static relocation models.

We intend to add lowering for at least GlobalISel at a later date, but this
is a temporary fix.

[arsenm]

Is the fallback not working correctly in this case for some reason?

[hctim]

In D82249#2105036, @arsenm wrote:

Is the fallback not working correctly in this case for some reason?

I'm fairly sure that G_GLOBAL_VALUE used to fallback onto SelectionDAGISel, and that was changed in D78465. Because we only implemented the custom adrp+movk lowering in SelectionDAGISel, making that instruction sequence no longer fallback, so we don't get our proper lowering for HWASan globals.

[eugenis]

LGTM

I'm OK with this as a workaround, but it would be more natural to detect the unsupported IR pattern in globalisel and fall back instead of disabling it entirely. Is it difficult to do for some reason?

[arsenm]

In D82249#2107845, @hctim wrote:

In D82249#2105036, @arsenm wrote:

Is the fallback not working correctly in this case for some reason?

I'm fairly sure that G_GLOBAL_VALUE used to fallback onto SelectionDAGISel, and that was changed in D78465. Because we only implemented the custom adrp+movk lowering in SelectionDAGISel, making that instruction sequence no longer fallback, so we don't get our proper lowering for HWASan globals.

I don't follow. It no longer falls back, so what is the problem?

[hctim]

In D82249#2109920, @eugenis wrote:

I'm OK with this as a workaround, but it would be more natural to detect the unsupported IR pattern in globalisel and fall back instead of disabling it entirely. Is it difficult to do for some reason?

Eh, it's not an unsupported IR pattern that's the problem, it's that the IR is lowered into adrp + add so that the add can be folded into a ldr/str as an offset. IMO on the scale of "painting over the problem vs. fixing it", this patch is 100% paint, forced fallback with MO_TAGGED is 80% paint for maybe 60% of the work of just fixing it.

I'm working on fixing this properly now that we know we don't need to make a less-risky, fast-to-deploy patch. If that all falls into place this patchset will just become obsolete anyway.

[eugenis]

In D82249#2110054, @hctim wrote:

In D82249#2109920, @eugenis wrote:

I'm OK with this as a workaround, but it would be more natural to detect the unsupported IR pattern in globalisel and fall back instead of disabling it entirely. Is it difficult to do for some reason?

Eh, it's not an unsupported IR pattern that's the problem, it's that the IR is lowered into adrp + add so that the add can be folded into a ldr/str as an offset. IMO on the scale of "painting over the problem vs. fixing it", this patch is 100% paint, forced fallback with MO_TAGGED is 80% paint for maybe 60% of the work of just fixing it.

I'm working on fixing this properly now that we know we don't need to make a less-risky, fast-to-deploy patch. If that all falls into place this patchset will just become obsolete anyway.

Right, well, there is an IR pattern that is not handled correctly in globalisel. Let's call it "unsupported". A better way to work around that is to detect this pattern in globalisel and fall back.

If you can fix the underlying problem, that's even better, of course.

[hctim]

In D82249#2110010, @arsenm wrote:

I don't follow. It no longer falls back, so what is the problem?

HWASan-globals end up with an address that's outside of the code model (due to the tag), so the normal instruction sequence of adrp + mov or adrp + ldr/str (with folded imm) isn't adequate. In SelectionDAGISel, we emit a movk as well after the adrp to capture the top 16 bits. We need to do the same for GlobalISel. This is a temporary workaround to fix HWASan at -O0 while I go and add support for adding the tagged-address lowering in GlobalISel.

[hctim]

Abandoning - fixing the underlying issue at D82615.

clang/lib/Driver/SanitizerArgs.cpp

void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
   if (!HwasanAbi.empty()) {
     CmdArgs.push_back("-default-function-attr");
     CmdArgs.push_back(Args.MakeArgString("hwasan-abi=" + HwasanAbi));
   }
 
   if (Sanitizers.has(SanitizerKind::HWAddress)) {
     CmdArgs.push_back("-target-feature");
     CmdArgs.push_back("+tagged-globals");
+
+    // Workaround for HWASan not implementing the +tagged-globals feature
+    // properly in GlobalISel, which is enabled by default at -O0 for aarch64.
+    // Once GlobalISel is disabled, -O0 uses FastISel, so we disable that as
+    // well to ensure we get SelectionDagISel where the feature is properly
+    // implemented.
+    // TODO(hctim): Implement proper ISel lowering for taking the address of
+    // tagged globals in at *least* GlobalISel. Probably okay to ignore FastISel
+    // as it's not part of any defaults set.
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back("--aarch64-enable-global-isel-at-O=-1"); // No GlobalISel.
+
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back("-fast-isel=false");
   }
 
   // MSan: Workaround for PR16386.
   // ASan: This is mainly to help LSan with cases such as
   // https://github.com/google/sanitizers/issues/373
   // We can't make this conditional on -fsanitize=leak, as that flag shouldn't
   // affect compilation.
   if (Sanitizers.has(SanitizerKind::Memory) ||

clang/test/Driver/fsanitize.c

 // CHECK-SCUDO-KMSAN: error: invalid argument '-fsanitize=kernel-memory' not allowed with '-fsanitize=scudo'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=hwaddress %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-HWASAN-INTERCEPTOR-ABI
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=hwaddress -fsanitize-hwaddress-abi=interceptor %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-HWASAN-INTERCEPTOR-ABI
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=hwaddress -fsanitize-hwaddress-abi=platform %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-HWASAN-PLATFORM-ABI
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=hwaddress -fsanitize-hwaddress-abi=foo %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-HWASAN-FOO-ABI
 // CHECK-HWASAN-INTERCEPTOR-ABI: "-default-function-attr" "hwasan-abi=interceptor"
 // CHECK-HWASAN-INTERCEPTOR-ABI: "-target-feature" "+tagged-globals"
+// CHECK-HWASAN-INTERCEPTOR-ABI: "-mllvm" "--aarch64-enable-global-isel-at-O=-1"
+// CHECK-HWASAN-INTERCEPTOR-ABI: "-mllvm" "-fast-isel=false"
 // CHECK-HWASAN-PLATFORM-ABI: "-default-function-attr" "hwasan-abi=platform"
 // CHECK-HWASAN-PLATFORM-ABI: "-target-feature" "+tagged-globals"
+// CHECK-HWASAN-PLATFORM-ABI: "-mllvm" "--aarch64-enable-global-isel-at-O=-1"
+// CHECK-HWASAN-PLATFORM-ABI: "-mllvm" "-fast-isel=false"
 // CHECK-HWASAN-FOO-ABI: error: invalid value 'foo' in '-fsanitize-hwaddress-abi=foo'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address,pointer-compare,pointer-subtract %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POINTER-ALL
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=pointer-compare %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POINTER-CMP-NEEDS-ADDRESS
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=pointer-subtract %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POINTER-SUB-NEEDS-ADDRESS
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=pointer-subtract -fno-sanitize=pointer-subtract %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-POINTER-SUB
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=pointer-compare -fno-sanitize=pointer-compare %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-POINTER-CMP
 // CHECK-POINTER-ALL: -cc1{{.*}}-fsanitize={{[^"]*}}pointer-compare,pointer-subtract{{.*}}" {{.*}} "-mllvm" "-asan-detect-invalid-pointer-cmp" {{.*}}"-mllvm" "-asan-detect-invalid-pointer-sub"

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
+
+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/test/CodeGen/AArch64/tagged-globals.ll

-; RUN: llc < %s | FileCheck %s
+; RUN: llc --relocation-model=static < %s | FileCheck %s
+; RUN: llc --relocation-model=pic < %s | 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
+  ; CHECK: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK: movk [[REG]], #:prel_g3:global+4294967296
+  ; CHECK: add x0, [[REG]], :lo12:global
+  ; CHECK: 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: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK: ldr w0, {{\[}}[[REG]], :lo12:global{{\]}}
+  ; CHECK: 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: global_store:
+  ; CHECK: adrp [[REG:x[0-9]+]], :pg_hi21_nc:global
+  ; CHECK: str wzr, {{\[}}[[REG]], :lo12:global{{\]}}
+  ; CHECK: 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: adrp [[REG:x[0-9]+]], func
+  ; CHECK: add x0, [[REG]], :lo12:func
+  ; CHECK: 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" }