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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc 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.
 
----------------
eugenis wrote:
> 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.
Yes, it's wrong, I've fixed it.


================
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
----------------
eugenis wrote:
> Would it be useful to test values of SIZE near 0?
I added a test for SIZE=2. (SIZE=1 failed to detect an error to my surprise, but I realised that it was because deterministic tagging was giving us a tag of 1 which was equal to the short tag.)


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:623
+        {shadowBase(), Ptr,
+         ConstantInt::get(Int32Ty, AccessInfo + UseShortGranules * 0x40)});
     return;
----------------
eugenis wrote:
> 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).
Yes, adding a new intrinsic seems like the best option if we want to keep the AccessInfo format the same everywhere. Done.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:644
+  Instruction *CheckTerm = SplitBlockAndInsertIfThen(
+      TagMismatch, InsertBefore, !UseShortGranules && !Recover,
+      MDBuilder(*C).createBranchWeights(1, 100000));
----------------
eugenis wrote:
> 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 ?
The condition here is based on the diff from D63908. Recovery is always based on the `Recover` flag, but in the short granule case the unreachable block is created on line 652 instead.

That said, maybe it would be better to not change this part of the code at all, as this would let short granules work in code compiled with new compilers targeting old API levels running on new systems, even when we end up emitting an inline check. That's what I've now done.


================
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
----------------
eugenis wrote:
> I'm not sure why the number in __hwasan_check_* went down when you added more stuff into it. Are you matching partial string?
It's because I changed the value of `AccessInfo` so that I could test the emission of the check both with and without short granules. I ended up adjusting this code again for the new intrinsic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68059