[hwasan] Prevent reordering of tag checks.

[fmayer]

Summary

They were previously unconstrained, which allowed them to be reordered
before the shadow memory write.

[fmayer]

commit message change

[fmayer]

I ran 50 iterations of pdfium_test with baseline vs. this change and could see no statistically significant change (p-value is 88%): https://colab.research.google.com/drive/1BBOT0BYGnvS9bvbryMNyAsVBbVM30gx5#scrollTo=cfis4f2vI305

[eugenis]

Is mayStore=1 necessary? It does not make any sense, these instruction do not write to memory.

[fmayer]

only mayload

[fmayer]

In D107901#2940403, @eugenis wrote:

Is mayStore=1 necessary? It does not make any sense, these instruction do not write to memory.

That also works. I don't think it's necessary to re-run the benchmarks as it's not expected that things are worse like this.

[eugenis]

It would be nice to have a test for this, maybe something like test/CodeGen/AArch64/irg-nomem.mir (but the opposite - you'd want store merging to not happen).

Another weird thing: even without this patch I see

{ 561,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList4, ImplicitList5, OperandInfo65 },  // Inst #561 = HWASAN_CHECK_MEMACCESS
{ 562,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList6, ImplicitList5, OperandInfo65 },  // Inst #562 = HWASAN_CHECK_MEMACCESS_SHORTGRANULES

in lib/Target/AArch64/AArch64GenInstrInfo.inc, i.e. these instructions are already mayLoad | mayStore.

[eugenis]

I find this program useful to test aliasing behavior:

void zzz(long *x) {
  x[0] = 42;
  x[1] = 42;
}

With -fno-slp-vectorize it generates store - check - store sequence where two stores can be merged into store-pair (see AArch64LoadStoreOptimizer) if hwasan check is not mayLoadOrStore.

[fmayer]

In D107901#2942065, @eugenis wrote:

Another weird thing: even without this patch I see

{ 561,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList4, ImplicitList5, OperandInfo65 },  // Inst #561 = HWASAN_CHECK_MEMACCESS
{ 562,        2,      0,      0,      0,      0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::MayStore), 0x0ULL, ImplicitList6, ImplicitList5, OperandInfo65 },  // Inst #562 = HWASAN_CHECK_MEMACCESS_SHORTGRANULES

in lib/Target/AArch64/AArch64GenInstrInfo.inc, i.e. these instructions are already mayLoad | mayStore.

I tried to revert this part and it still solves the issues. I would still prefer to leave it as is to have it more explicit.

[fmayer]

revert unnecessary change

[fmayer]

add test

[fmayer]

add missing newline

[fmayer]

I verified this test fails at baseline and passes with this change.

[eugenis]

LGTM

Please check that the test works in release-without-assertions build. It tends to loose names of temporaries like "%x.hwasan".

[fmayer]

In D107901#2947723, @eugenis wrote:

LGTM

Please check that the test works in release-without-assertions build. It tends to loose names of temporaries like "%x.hwasan".

passes with -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=0

[aeubanks]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3

this is the only user of print-memdeps which is a legacy PM-specific pass. -analyze is also a legacy PM-specific feature.
is there a better way of testing this? memdep is deprecated anyway

[fmayer]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3

I don't know of any more direct way to test this. Is there in the new PM that you know about that can give information about memory dependencies / what can't be reordered?

[aeubanks]

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

3

D119393

llvm/include/llvm/IR/Intrinsics.td

 // callees. This needs to be a musttail call.
 def int_icall_branch_funnel : DefaultAttrsIntrinsic<[], [llvm_vararg_ty], []>;
 
 def int_load_relative: DefaultAttrsIntrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_anyint_ty],
                                  [IntrReadMem, IntrArgMemOnly]>;
 
 def int_hwasan_check_memaccess :
   Intrinsic<[], [llvm_ptr_ty, llvm_ptr_ty, llvm_i32_ty],
-            [IntrInaccessibleMemOnly, ImmArg<ArgIndex<2>>]>;
+            [ImmArg<ArgIndex<2>>]>;
 def int_hwasan_check_memaccess_shortgranules :
   Intrinsic<[], [llvm_ptr_ty, llvm_ptr_ty, llvm_i32_ty],
-            [IntrInaccessibleMemOnly, ImmArg<ArgIndex<2>>]>;
+            [ImmArg<ArgIndex<2>>]>;
 
 // Xray intrinsics
 //===----------------------------------------------------------------------===//
 // Custom event logging for x-ray.
 // Takes a pointer to a string and the length of the string.
 def int_xray_customevent : Intrinsic<[], [llvm_ptr_ty, llvm_i32_ty],
                                      [IntrWriteMem, NoCapture<ArgIndex<0>>,
                                       ReadOnly<ArgIndex<0>>]>;

llvm/test/Instrumentation/HWAddressSanitizer/memaccess-clobber.ll

+; Make sure memaccess checks preceed the following reads.
+;
+; RUN: opt < %s -S -enable-new-pm=0 -hwasan -basic-aa -memdep -print-memdeps -analyze -mtriple aarch64-linux-android30 | FileCheck %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-android10000"
+
+declare void @use32(i32*)
+
+define i32 @test_alloca() sanitize_hwaddress {
+entry:
+  %x = alloca i32, align 4
+  call void @use32(i32* nonnull %x)
+  ; CHECK: Clobber from:   call void @llvm.hwasan.check.memaccess.shortgranule
+  ; CHECK-NEXT: load i32, i32* %x.hwasan, align 4
+  %y = load i32, i32* %x
+  ; CHECK:  Clobber from:   %y = load i32, i32* %x.hwasan, align 4
+  ; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* align 1 {{.*}}, i8 0, i64 1, i1 false)
+  ret i32 %y
+}