[scudo] Enabling MIPS support for Scudo

[slthakur]

Summary

Adding MIPS 32-bit and 64-bit support for Scudo.

[cryptoad]

Thank you for this. I am curious, what did you test this on?
I have no experience on MIPS, would you be able to provide some help in case of any breakage?

[slthakur]

Hi,

Thank you for this. I am curious, what did you test this on?
I have no experience on MIPS, would you be able to provide some help in case of any breakage?

I have tested this on a mips64 little endian machine. All tests are passing.
Yes, I will be happy to help if there is any breakage.

Regards,
Sagar

[cryptoad]

LGTM

[slthakur]

Committed in revision 301158.

[sdardis]

Sagar, rL301158 broke the buildbot clang-cmake-mips starting with http://lab.llvm.org:8011/builders/clang-cmake-mips/builds/3179 . Can you investigate?

[slthakur]

Hi Simon,

I am investigating the failure. Meanwhile I'll revert the commit.

Regards,
Sagar

[slthakur]

Added 64-bit atomic functions for mips32 which are required in sanitizer common.

We needed to add these function because we were getting undefined references to sync_fetch_and_add_8, sync_val_compare_and_swap_8 functions.

[cryptoad]

Still LGTM for the Scudo side of things, but not being familiar with MIPS and the platform in general, I don't think I can provide appropriate feedback on the atomic modifications done in sanitizer_common.
If someone else could have a look at it, that would be great.

[sdardis]

This appears functionally correct, but really requires documentation explaining the implementation.

My other concern is that having a single global lock for all atomic functions may be something of a bottleneck in terms of performance. The lock could be duplicated on a per template basis to avoid unnecessary contention but I'd really prefer if we could simply use template specialisation to handle 64bit types.

lib/sanitizer_common/sanitizer_atomic_clang_other.h

20

This preprocessor define check I believe to too broad.

Instead it should be:

#if defined(_MIPS_SIM) && _MIPS_SIM == _ABIO32

As it's O32 that cannot use 64 bit atomics.

29–32

This structure definition is strange.

I am guessing that you're allocating a typical MIPS sized cache into the data section, with a lock and padding, the purpose of the padding to avoid spurious ll/sc failures due to writes near the lock.

I understand that you're using a single global lock for __mips_sync_* , so that you can update types larger than a word on MIPS32 platforms, as they do not have larger than word sized atomic updates.

This hunk really needs a (long) comment to explain the reasoning and implementation.

[slthakur]

Addressed review comments

[slthakur]

Using preprocessor macro _MIPS_SIM in sanitizer_atomic_clang.h as well.

[sdardis]

LGTM with inline comments addressed.

Please add comments for the bug addition to lib/sanitizer_common/sanitizer_atomic_clang_other.h, other maintainers will not necessarily know why MIPS requires an internal spinlock for 8 byte atomics.

lib/sanitizer_common/sanitizer_atomic_clang.h

84

The casts of (u64) for cmpv and xchg should be reinterpret_cast<u64> rather than C-style casts. C style casts trigger conversions whereas we want to reinterpret the bit pattern.

lib/sanitizer_common/sanitizer_atomic_clang_other.h

20

Prefix this hunk with a comment saying that MIPS32 or code compiled for the O32 ABI on MIPS does not support atomics > 4 bytes. To address this lack of functionality, the sanitizer library provides helper methods which use an internal spin lock mechanism to emulate atomic operations when the size is 8 bytes.

148

reinterpret_cast<u64> rather than (u64).

[slthakur]

Addressed review comments.

[slthakur]

Committed in revision 305682.
Thanks to Simon for the review and for fixing the build breakage.

cmake/config-ix.cmake

 set(ALL_PROFILE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${PPC64}
     ${MIPS32} ${MIPS64} ${S390X})
 set(ALL_TSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${PPC64})
 set(ALL_UBSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64}
     ${MIPS32} ${MIPS64} ${PPC64} ${S390X})
 set(ALL_SAFESTACK_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${MIPS32} ${MIPS64})
 set(ALL_CFI_SUPPORTED_ARCH ${X86} ${X86_64} ${MIPS64})
 set(ALL_ESAN_SUPPORTED_ARCH ${X86_64} ${MIPS64})
-set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64})
+set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64})
 set(ALL_XRAY_SUPPORTED_ARCH ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} powerpc64le)
 
 if(APPLE)
   include(CompilerRTDarwinUtils)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)

lib/sanitizer_common/sanitizer_atomic_clang.h

INLINE typename T::Type atomic_exchange(volatile T *a,
   if (mo & (memory_order_release | memory_order_acq_rel | memory_order_seq_cst))
     __sync_synchronize();
   v = __sync_lock_test_and_set(&a->val_dont_use, v);
   if (mo == memory_order_seq_cst)
     __sync_synchronize();
   return v;
 }
 
-template<typename T>
-INLINE bool atomic_compare_exchange_strong(volatile T *a,
-                                           typename T::Type *cmp,
+template <typename T>
+INLINE bool atomic_compare_exchange_strong(volatile T *a, typename T::Type *cmp,
                                            typename T::Type xchg,
                                            memory_order mo) {
   typedef typename T::Type Type;
   Type cmpv = *cmp;
-  Type prev = __sync_val_compare_and_swap(&a->val_dont_use, cmpv, xchg);
-  if (prev == cmpv)
-    return true;
+  Type prev;
+#if defined(_MIPS_SIM) && _MIPS_SIM == _ABIO32
+  if (sizeof(*a) == 8) {
+    Type volatile *val_ptr = const_cast<Type volatile *>(&a->val_dont_use);
+    prev = __mips_sync_val_compare_and_swap<u64>(
+        reinterpret_cast<u64 volatile *>(val_ptr), reinterpret_cast<u64> cmpv,
+        reinterpret_cast<u64> xchg);
+  } else {
+    prev = __sync_val_compare_and_swap(&a->val_dont_use, cmpv, xchg);
+  }
+#else
+  prev = __sync_val_compare_and_swap(&a->val_dont_use, cmpv, xchg);
+#endif
+  if (prev == cmpv) return true;
   *cmp = prev;
   return false;
 }
 
 template<typename T>
 INLINE bool atomic_compare_exchange_weak(volatile T *a,
                                          typename T::Type *cmp,
                                          typename T::Type xchg,

lib/sanitizer_common/sanitizer_atomic_clang_other.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SANITIZER_ATOMIC_CLANG_OTHER_H
 #define SANITIZER_ATOMIC_CLANG_OTHER_H
 
 namespace __sanitizer {
 
+// MIPS32 does not support atomic > 4 bytes. To address this lack of
+// functionality, the sanitizer library provides helper methods which use an
+// internal spin lock mechanism to emulate atomic oprations when the size is
+// 8 bytes.
+#if defined(_MIPS_SIM) && _MIPS_SIM == _ABIO32
+static void __spin_lock(volatile int *lock) {
+  while (__sync_lock_test_and_set(lock, 1))
+    while (*lock) {
+    }
+}
+
+static void __spin_unlock(volatile int *lock) { __sync_lock_release(lock); }
+
+
+// Make sure the lock is on its own cache line to prevent false sharing.
+// Put it inside a struct that is aligned and padded to the typical MIPS
+// cacheline which is 32 bytes.
+static struct {
+  int lock;
+  char pad[32 - sizeof(int)];
+} __attribute__((aligned(32))) lock = {0};
+
+template <class T>
+T __mips_sync_fetch_and_add(volatile T *ptr, T val) {
+  T ret;
+
+  __spin_lock(&lock.lock);
+
+  ret = *ptr;
+  *ptr = ret + val;
+
+  __spin_unlock(&lock.lock);
+
+  return ret;
+}
+
+template <class T>
+T __mips_sync_val_compare_and_swap(volatile T *ptr, T oldval, T newval) {
+  T ret;
+  __spin_lock(&lock.lock);
+
+  ret = *ptr;
+  if (ret == oldval) *ptr = newval;
+
+  __spin_unlock(&lock.lock);
+
+  return ret;
+}
+#endif
+
 INLINE void proc_yield(int cnt) {
   __asm__ __volatile__("" ::: "memory");
 }
 
 template<typename T>
 INLINE typename T::Type atomic_load(
     const volatile T *a, memory_order mo) {
   DCHECK(mo & (memory_order_relaxed | memory_order_consume
if (mo == memory_order_relaxed) {
       __sync_synchronize();
       v = a->val_dont_use;
       __sync_synchronize();
     }
   } else {
     // 64-bit load on 32-bit platform.
     // Gross, but simple and reliable.
     // Assume that it is not in read-only memory.
+#if defined(_MIPS_SIM) && _MIPS_SIM == _ABIO32
+    typename T::Type volatile *val_ptr =
+        const_cast<typename T::Type volatile *>(&a->val_dont_use);
+    v = __mips_sync_fetch_and_add<u64>(
+        reinterpret_cast<u64 volatile *>(val_ptr), 0);
+#else
     v = __sync_fetch_and_add(
         const_cast<typename T::Type volatile *>(&a->val_dont_use), 0);
+#endif
   }
   return v;
 }
 
 template<typename T>
 INLINE void atomic_store(volatile T *a, typename T::Type v, memory_order mo) {
   DCHECK(mo & (memory_order_relaxed | memory_order_release
       | memory_order_seq_cst));
if (mo == memory_order_relaxed) {
       __sync_synchronize();
     }
   } else {
     // 64-bit store on 32-bit platform.
     // Gross, but simple and reliable.
     typename T::Type cmp = a->val_dont_use;
     typename T::Type cur;
     for (;;) {
+#if defined(_MIPS_SIM) && _MIPS_SIM == _ABIO32
+      typename T::Type volatile *val_ptr =
+          const_cast<typename T::Type volatile *>(&a->val_dont_use);
+      cur = __mips_sync_val_compare_and_swap<u64>(
+          reinterpret_cast<u64 volatile *>(val_ptr), reinterpret_cast<u64> cmp,
+          reinterpret_cast<u64> v);
+#else
       cur = __sync_val_compare_and_swap(&a->val_dont_use, cmp, v);
+#endif
       if (cmp == v)
         break;
       cmp = cur;
     }
   }
 }
 
 }  // namespace __sanitizer
 
 #endif  // #ifndef SANITIZER_ATOMIC_CLANG_OTHER_H

test/scudo/random_shuffle.cpp

 // RUN: %clang_scudo %s -o %t
 // RUN: rm -rf %T/random_shuffle_tmp_dir
 // RUN: mkdir %T/random_shuffle_tmp_dir
 // RUN: %run %t 100 > %T/random_shuffle_tmp_dir/out1
 // RUN: %run %t 100 > %T/random_shuffle_tmp_dir/out2
 // RUN: %run %t 10000 > %T/random_shuffle_tmp_dir/out1
 // RUN: %run %t 10000 > %T/random_shuffle_tmp_dir/out2
 // RUN: not diff %T/random_shuffle_tmp_dir/out?
 // RUN: rm -rf %T/random_shuffle_tmp_dir
-// UNSUPPORTED: i386-linux,i686-linux,arm-linux,armhf-linux,aarch64-linux
+// UNSUPPORTED: i386-linux,i686-linux,arm-linux,armhf-linux,aarch64-linux,mips-linux,mipsel-linux,mips64-linux,mips64el-linux
 
 // Tests that the allocator shuffles the chunks before returning to the user.
 
 #include <stdlib.h>
 #include <stdio.h>
 
 int main(int argc, char **argv) {
   int alloc_size = argc == 2 ? atoi(argv[1]) : 100;
   char *base = new char[alloc_size];
   for (int i = 0; i < 20; i++) {
     char *p = new char[alloc_size];
     printf("%zd\n", base - p);
   }
 }