[PATCH] D68059: hwasan: Compatibility fixes for short granules.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S:55
 //   * x0: The address of read/write instruction that caused HWASan check fail.
 //   * x1: The tag size.
 
----------------
This comment is wrong, isn't it? X1 is the encoded access info, which includes size as well as a read/write bit and more. Could you fix this please?

And x0 is not the address of an instruction. It's the data address.


================
Comment at: compiler-rt/test/hwasan/TestCases/stack-oob.c:3
 // RUN: %clang_hwasan -DSIZE=15 -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clang_hwasan_oldrt -DSIZE=16 -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
 // RUN: %clang_hwasan -DSIZE=16 -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
----------------
Would it be useful to test values of SIZE near 0?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:623
+        {shadowBase(), Ptr,
+         ConstantInt::get(Int32Ty, AccessInfo + UseShortGranules * 0x40)});
     return;
----------------
Do you think this is better expressed as a new intrinsic? I don't like UseShortGranules mixed into AccessInfo, which has lots of stuff encoded in it, but at least it has the same format everywhere (compiler & runtime library).


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:644
+  Instruction *CheckTerm = SplitBlockAndInsertIfThen(
+      TagMismatch, InsertBefore, !UseShortGranules && !Recover,
+      MDBuilder(*C).createBranchWeights(1, 100000));
----------------
Is this condition right? If not using short granules, an access can be bad from the point of view of this code, but good from the runtime library POV and we might get control back.

Should this be UseShortGranules && !Recover ?


================
Comment at: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll:11
   ; CHECK-NEXT: mov x9, x0
-  ; CHECK-NEXT: bl __hwasan_check_x1_123
+  ; CHECK-NEXT: bl __hwasan_check_x1_1
   ; CHECK-NEXT: mov x0, x1
----------------
I'm not sure why the number in __hwasan_check_* went down when you added more stuff into it. Are you matching partial string?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68059/new/

https://reviews.llvm.org/D68059