[PATCH] [lsan] [mips] adding support of lsan for mips64/mips64el arch

Sergey Matveev earthdok at google.com
Fri Oct 24 13:41:58 PDT 2014


How did you test this patch?

>>! In D5616#23, @kumarsukhani wrote:
> @earthdok: whether we should check for '__mips64__' instead of '__mips__' as we are anyways not supporting 32-bit mips for LSAN?

That would be more consistent, yes (we do check for __x86_64__).
Is __mips64__ the right symbol? __mips64 seems to be more prevalent, and https://code.google.com/p/android/issues/detail?id=75904 suggests __mips64__ may not be defined when building with gcc.

================
Comment at: lib/lsan/lsan_allocator.cc:29
@@ -28,1 +28,3 @@
 static const uptr kMaxAllowedMallocSize = 8UL << 30;
+#if defined(__mips__) && (SANITIZER_WORDSIZE == 64)
+static const uptr kAllocatorSpace = 0x4000000000ULL;
----------------
(SANITIZER_WORDSIZE == 64) is unnecessary, please remove it.

================
Comment at: lib/lsan/lsan_common.cc:147
@@ -146,1 +146,3 @@
   return ((p >> 47) == 0);
+#elif defined(__mips__) && (SANITIZER_WORDSIZE == 64)
+  return ((p >> 40) == 0);
----------------
please remove (SANITIZER_WORDSIZE == 64)

================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:841
@@ -836,1 +840,3 @@
+               newtls, child_tidptr);
+}
 #endif  // defined(__x86_64__) && SANITIZER_LINUX
----------------
kumarsukhani wrote:
> @earthdok: After the basic support for mips64 is merged, I will start working on implementing clone in assembly.
> Btw using syscall(clone,..) is not a good option for this?
> 
Please leave a TODO, then.

> Btw using syscall(clone,..) is not a good option for this?

It wouldn't work at all. We'd crash when returning from syscall() in the child process, because we're now in the beginning of a new stack and there isn't a previous stack frame to return to.

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:236
@@ +235,3 @@
+# elif defined(__mips__)
+  char *ptr;
+
----------------
Why do you need this? Just write to descr_addr.

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:242
@@ +241,3 @@
+
+#define TLS_TCB_OFFSET       0x7000
+#define TLS_PRE_TCB_SIZE       0x10
----------------
const uptr kTlsTcbOffset
const uptr kTlsPreTcbSize

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:287
@@ +286,3 @@
+# elif defined(__mips__)
+  *addr = ThreadSelf();
+  *size = GetTlsSize();
----------------
Is this correct? I thought in variant 2 the static TLS was located before the TCB?

http://reviews.llvm.org/D5616






More information about the llvm-commits mailing list