[PATCH] D114558: [ASan] Shared optimized callbacks implementation.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

I would recommend to separate llvm into a separate patch, so you can land compiler-rt separately. That part is likely NFC on it's own, but you can wait some time to see if it was not reverted because of breaking. And then you can land/revert llvm part as many times you want



================
Comment at: compiler-rt/lib/asan/asan_mapping.h:160-225
 static const u64 kDefaultShadowOffset32 = 1ULL << 29;  // 0x20000000
-static const u64 kDefaultShadowOffset64 = 1ULL << 44;
+static const u64 kDefaultShadowOffset64 = 1ULL << 44;  // 0x100000000000
 static const u64 kDefaultShort64bitShadowOffset =
     0x7FFFFFFF & (~0xFFFULL << kDefaultShadowScale);  // < 2G.
-static const u64 kAArch64_ShadowOffset64 = 1ULL << 36;
+static const u64 kAArch64_ShadowOffset64 = 1ULL << 36;  // 0x1000000000
 static const u64 kRiscv64_ShadowOffset64 = 0xd55550000;
 static const u64 kMIPS32_ShadowOffset32 = 0x0aaa0000;
----------------
it would be nice to move formating out of the patch and submit separately, review is not required


================
Comment at: compiler-rt/lib/asan/asan_rtl_x86_64.S:88
+
+// Access check functions for 8 and 16 bytwe types: no extra checks required.
+#define ASAN_MEMORY_ACCESS_CHECK_ADD(reg, op, s, c) \
----------------
morehouse wrote:
> 
it's back again?


================
Comment at: compiler-rt/lib/asan/asan_shadow_defines.h:17
+#if !defined(SANITIZER_WORDSIZE)
+#  error "SANITIZER_WORDSIZE should be defined."
+#endif
----------------
why can't you move this into asan_mapping.h?


================
Comment at: compiler-rt/lib/asan/tests/asan_noinst_test.cpp:331
+// R8 call back.
+ASAN_MEMORY_ACCESS_CALLBACKS_ADD(RAX)
+ASAN_MEMORY_ACCESS_CALLBACKS_ADD(RBX)
----------------
ASAN_MEMORY_ACCESS_CALLBACKS_ADD -> TEST_ASAN_MEMORY_ACCESS_CALLBACKS_ADD

because it instantiates tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114558