[PATCH] AArch64 sanitizer support

Kostya Serebryany kcc at google.com
Mon Feb 10 21:11:39 PST 2014


  This patch is obviously based on the GCC source tree, LLVM source does not have 'libsanitizer' directory or a file configure.tgt.
  Let me give the first round of comments here, but then please send another patch based on the LLMV tree.

  >> As I work on GCC, I have written the patch and tested it within GCC,

  Note that the GCC test suit for ASAN is much weaker than the one in LLVM.
  I'd suggest you to test against the LLVM test suite (not necessary before the first commit).
  Does LLVM already support AArch64 to the degree where asan tests could be run?


================
Comment at: libsanitizer/asan/asan_allocator2.cc:63
@@ +62,3 @@
+const uptr kAllocatorSpace =  0x4000000000ULL; // 500 GB
+const uptr kAllocatorSize  =  0x2000000000ULL; // 200 GB
+#  else
----------------
200Gb is not enough. 
If you give the allocator 200Gb, it means that every size class is getting 200/64 = 3.2Gb,
i.e. asan will not work on large apps. 
Given how small is the address space on AArch64 (btw, why is it so small??),
we need to use SizeClassAllocator32 instead of SizeClassAllocator64, 
and then you should not need these macros. 
In the same patch I see you define SANITIZER_CAN_USE_ALLOCATOR64 to 0. 
So you can just drop this part.

================
Comment at: libsanitizer/asan/asan_mapping.h:63
@@ -62,2 +62,3 @@
 static const u64 kPPC64_ShadowOffset64 = 1ULL << 41;
+static const u64 kAArch64_ShadowOffset64 = 1ULL << 36;
 static const u64 kMIPS32_ShadowOffset32 = 0x0aaa8000;
----------------
Note that on x86 we use 0x7FFF8000 (a small offset that fits into smaller instructions)
as a performance optimization. 
I don't know AArch64 well enough to judge whether a smaller offset will give us performance benefits.
Any ideas? 

================
Comment at: libsanitizer/asan/asan_rtl.cc:487
@@ -485,1 +486,3 @@
     kMidMemEnd = kLowMemEnd < 0x3000000000ULL ? 0x4fffffffffULL : 0;
+# else
+#  if defined(__aarch64__)
----------------
Please use #elif
But: is this section at all relevant for AArch64? 
This is here for the linux prelink feature -- I am not sure if it is present on non-x86_64.
Unless you have a test that depends on this part of patch -- please drop it. 

================
Comment at: libsanitizer/configure.tgt:35
@@ -34,1 +34,3 @@
 	;;
+  aarch64*-*-linux*)
+	TSAN_SUPPORTED=no
----------------
no such file in llvm

================
Comment at: libsanitizer/sanitizer_common/sanitizer_internal_defs.h:81
@@ -80,3 +80,3 @@
 
-#if (SANITIZER_WORDSIZE == 64) || SANITIZER_MAC
+#if (SANITIZER_WORDSIZE == 64) || SANITIZER_MAC || defined(__aarch64__)
 typedef uptr operator_new_size_type;
----------------
Isn't SANITIZER_WORDSIZE == 64 when defined(__aarch64__) ? 

================
Comment at: libsanitizer/sanitizer_common/sanitizer_linux.cc:62
@@ -61,3 +61,3 @@
 // but it still needs to use 64-bit syscalls.
-#if defined(__x86_64__) || SANITIZER_WORDSIZE == 64
+#if defined(__x86_64__) || defined(__aarch64__) || SANITIZER_WORDSIZE == 64
 # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
----------------
Isn't SANITIZER_WORDSIZE == 64 when defined(__aarch64__) ?

================
Comment at: libsanitizer/sanitizer_common/sanitizer_platform.h:80
@@ -75,2 +79,3 @@
 // but will consume more memory for TwoLevelByteMap.
-#define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
+#if defined(__aarch64__)
+# define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 39)
----------------
(begging pardon for my ignorance)
Is __aarch64__ available in 32-bit mode? Or is it __arm__ then? 
Why not just
#define SANITIZER_MMAP_RANGE_SIZE (1ULL << 39) 
?

================
Comment at: libsanitizer/sanitizer_common/sanitizer_linux.cc:96
@@ -95,1 +95,3 @@
 uptr internal_open(const char *filename, int flags) {
+#ifdef __aarch64__
+  return internal_syscall(__NR_openat, AT_FDCWD, (uptr)filename, flags);
----------------
OMG, these ifdefs again... 
What is so special about AArch64 that it needs other syscalls
(where ARM/PowerPC/SPARC/MIPS doesn't need them)?
If there is no way to avoid these ifdefs, they at least deserve a top-level comment in this file. 



http://llvm-reviews.chandlerc.com/D2732



More information about the llvm-commits mailing list