[MSan] [MIPS] Adding support for MIPS64

[mohit.bhakkad]

Summary

Most of the changes are related to mips64 arch.

[kcc]

lib/msan/msan.cc

510 ↗

(On Diff #15238)

eugenis@, maybe we should simply drop this code?

lib/msan/msan_linux.cc

38

Why do you use

#if defined(__mips64__) && (SANITIZER_WORDSIZE == 64)

here and just

#if defined(__mips64__)

elsewhere?

Does mips64 imply SANITIZER_WORDSIZE == 64?
If so, just use mips64

lib/sanitizer_common/sanitizer_allocator.h

475 ↗

(On Diff #15238)

Did you already verify that this actually works?
For a 40-bit address space you may need to use SanitizerAllocator32 instead.

[mohit.bhakkad]

This revision resolves mips64 flag related issue

[mohit.bhakkad]

lib/msan/msan_linux.cc

38

https://code.google.com/p/android/issues/detail?id=75904 suggests that __mips64 is prevalent than __mips64__

lib/sanitizer_common/sanitizer_allocator.h

475 ↗

(On Diff #15238)

Yes, I have verified using both SanitizerAllocator32 and SanitizerAllocator64, which are giving same results. As SizeClassAllocator64 is more efficient, so we are using that.

[kcc]

lib/sanitizer_common/sanitizer_allocator.h

475 ↗

(On Diff #15238)

How exactly did you verify it?
Did you run any large programs that consume gigs of RAM?
I still doubt this will work.

[mohit.bhakkad]

How exactly did you verify it?
Did you run any large programs that consume gigs of RAM?
I still doubt this will work.

MIPS is generally used in embedded, which may not require gigs of RAM.
Till now, I have just ran the testcases on mips rpe-100 board, and majority of it are passing.
For real world testing, I have tried MSan on nano editor, which is also working fine.
I will be testing it with some larger application, and will update you with the results.
Any suggestions?

[samsonov]

I'm fine with adjusting constants for MIPS you're doing here, but will let Kostya sign this off. I hope you will be able to setup the buildbot, otherwise we can frequently break you.

lib/sanitizer_common/sanitizer_allocator.h

478 ↗

(On Diff #15477)

s/its/it's

[mohit.bhakkad]

@kcc @samsonov :Thanks for your comments.

To make MSan work on MIPS, there are 2 more patches:

• LLVM code: kShadowMask and kOriginOffset are to be defined for MIPS:

lib/Transforms/Instrumentation/MemorySanitizer.cpp#L123

• Clang code: to enable memory option for mips.

lib/Driver/SanitizerArgs.cpp#L272

I just wanted to ask if I should submit those patches for review, or wait till this one is merged.

[eugenis]

I'm OK with this change, but please wait for a comment from Kostya.

I just wanted to ask if I should submit those patches for review, or wait till this one is merged.

Better send them early.

lib/msan/msan.h

35

Please change this to #elif defined(x86_64) and an #error if neither is defined.
The same in msan_allocator.cc and other similar cases.

[mohit.bhakkad]

lib/msan/msan.h

35

I will put x86_64 in #elif, but an extra line for #error will be redundant, as control will never come here, as "memory" option is not enabled in clang driver for those architectures.

[mohit.bhakkad]

Changes in this revision:

1. Used SanitizerAllocator32 instead of SanitizerAllocator64 for mips64 as its address space is just 40-bit.
2. Changed increment for PC in origin tracking w.r.t. mips64.
3. Put x86_64 in #elif instead of just #else.

[samsonov]

lib/msan/msan_report.cc

56

Evgeniy, do we know the reason why this adjustment is needed? Looks like we'd have to use the inverse of StackTrace::GetPreviousInstructionPc() here.

lib/sanitizer_common/sanitizer_posix.cc

93

Hm? This looks similar to the previous #elif

[eugenis]

lib/msan/msan_report.cc

56

No. I'd like to know why :)

[mohit.bhakkad]

lib/sanitizer_common/sanitizer_posix.cc

93

Sorry for my mistake, I didn't realized that it was added in some previous patch.
Any other comments/changes on this revision?

[kcc]

lib/msan/msan.cc

510 ↗

(On Diff #16151)

This code will get deleted with http://reviews.llvm.org/D6268, please wait for it.

lib/msan/msan_report.cc

58

I'd prefer not to have #ifdef here.
At the very least, move it into a separate function near the StackTrace::GetPreviousInstructionPc
but I would like to understand why we need this adjustment here at all.

[mohit.bhakkad]

lib/msan/msan_report.cc

58

I don't know the reason for this adjustment.
I will submit a new revision with a function StackTrace::GetNextInstructionPc in
sanitizer_stacktrace.cc, once http://reviews.llvm.org/D6268 gets commited.

[mohit.bhakkad]

Changes in this revision:
Added function StackTrace::GetNextInstructionPc in sanitizer_stacktrace.cc

[kcc]

Looks good.
eugenis, please have one more look and commit if you are ok with this.

[mohit.bhakkad]

Thanks @kcc @eugenis @samsonov

Just a gentle reminder, that this patch is dependent on http://reviews.llvm.org/D6146 and http://reviews.llvm.org/D6147
I have one more query, is there is any plan to accept msan32?, if yes I will update whatever changes needed in http://reviews.llvm.org/D5672

[samsonov]

LGTM

[kcc]

I have one more query, is there is any plan to accept msan32?, if yes I will update whatever changes needed in http://reviews.llvm.org/D5672

msan on 32 bit will have very limited applicability due to limited address space.
Remember, the application will have only < 1Gb of RAM available to it.
Supporting msan on 32-bit is an additional maintenance cost with little to not benefit, at least where I see it.
Do you want msan on 32-bit because there are MIPS customers who really need it and can't use 64-bit,
or just for completeness?

[samsonov]

Landed in r222388.

[earthdok]

Now some lucky soul has to update all the docs. :)

[earthdok]

Hold on, there hasn't been an LGTM from eugenis@. Why did you land this?

[samsonov]

In D5906#14, @eugenis wrote:

I'm OK with this change, but please wait for a comment from Kostya.

^^ here ^^
eugenis@, did I land it too early?

We should update the docs after the change to LLVM instrumentation pass and to Clang driver are in.

[mohit.bhakkad]

In D5906#36, @kcc wrote:

msan on 32 bit will have very limited applicability due to limited address space.
Remember, the application will have only < 1Gb of RAM available to it.
Supporting msan on 32-bit is an additional maintenance cost with little to not benefit, at least where I see it.
Do you want msan on 32-bit because there are MIPS customers who really need it and can't use 64-bit,
or just for completeness?

We are aware of the limitation of MSan 32bit however there are existing customers of MIPS32 which we think might be potential users of MSAN 32bit.

[eugenis]

In D5906#42, @samsonov wrote:

In D5906#14, @eugenis wrote:

I'm OK with this change, but please wait for a comment from Kostya.

^^ here ^^
eugenis@, did I land it too early?

No, it's fine. LGTM.

[kcc]

We are aware of the limitation of MSan 32bit however there are existing customers of MIPS32 which we think might be potential users of MSAN 32bit.

Ok, let's try.
We can not afford to have MIIPS-32 and not have x86-32 because for most developers there is
no way to test on MIIPS-32.
I would ask you to enable MSan on x86-32, make sure that all tests pass,
that check-msan runs the 32-bit tests on x86-64 machines,
that the bots (lab.llvm.org:8011/builders/sanitizer-x86_64-linux and lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/)
run these tests too.
Then we can enable msan on other 32-bit architecture(s).

cmake/config-ix.cmake

 filter_available_targets(ASAN_SUPPORTED_ARCH
   x86_64 i386 i686 powerpc64 powerpc64le arm mips mipsel mips64 mips64el)
 filter_available_targets(DFSAN_SUPPORTED_ARCH x86_64)
 filter_available_targets(LSAN_SUPPORTED_ARCH x86_64)
 # LSan common files should be available on all architectures supported
 # by other sanitizers (even if they build into dummy object files).
 filter_available_targets(LSAN_COMMON_SUPPORTED_ARCH
   ${SANITIZER_COMMON_SUPPORTED_ARCH})
-filter_available_targets(MSAN_SUPPORTED_ARCH x86_64)
+filter_available_targets(MSAN_SUPPORTED_ARCH x86_64 mips64 mips64el)
 filter_available_targets(PROFILE_SUPPORTED_ARCH x86_64 i386 i686 arm mips mips64
   mipsel mips64el aarch64 powerpc64 powerpc64le)
 filter_available_targets(TSAN_SUPPORTED_ARCH x86_64)
 filter_available_targets(UBSAN_SUPPORTED_ARCH x86_64 i386 i686 arm aarch64 mips mipsel)
 
 if(ANDROID)
   set(OS_NAME "Android")
 else()

lib/msan/CMakeLists.txt

 append_list_if(COMPILER_RT_HAS_FPIE_FLAG -fPIE MSAN_RTL_CFLAGS)
 # Prevent clang from generating libc calls.
 append_list_if(COMPILER_RT_HAS_FFREESTANDING_FLAG -ffreestanding MSAN_RTL_CFLAGS)
 
 set(MSAN_RUNTIME_LIBRARIES)
 
 # Static runtime library.
 add_custom_target(msan)
-set(arch "x86_64")
-if(CAN_TARGET_${arch})
+foreach(arch ${MSAN_SUPPORTED_ARCH})
   add_compiler_rt_runtime(clang_rt.msan-${arch} ${arch} STATIC
     SOURCES ${MSAN_RTL_SOURCES}
             $<TARGET_OBJECTS:RTInterception.${arch}>
             $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>
             $<TARGET_OBJECTS:RTSanitizerCommonLibc.${arch}>
     CFLAGS ${MSAN_RTL_CFLAGS})
   add_dependencies(msan clang_rt.msan-${arch})
   list(APPEND MSAN_RUNTIME_LIBRARIES clang_rt.msan-${arch})
   if(UNIX)
     add_sanitizer_rt_symbols(clang_rt.msan-${arch} msan.syms.extra)
     add_dependencies(msan clang_rt.msan-${arch}-symbols)
   endif()
-endif()
+endforeach()
 
 add_compiler_rt_resource_file(msan_blacklist msan_blacklist.txt)
 add_dependencies(msan msan_blacklist)
 add_dependencies(compiler-rt msan)
 
 if(COMPILER_RT_INCLUDE_TESTS)
   add_subdirectory(tests)
 endif()

lib/msan/msan.h

 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "msan_interface_internal.h"
 #include "msan_flags.h"
 
 #ifndef MSAN_REPLACE_OPERATORS_NEW_AND_DELETE
 # define MSAN_REPLACE_OPERATORS_NEW_AND_DELETE 1
 #endif
 
+#if defined(__mips64)
+#define MEM_TO_SHADOW(mem)       (((uptr)mem) & ~0x4000000000ULL)
+#define SHADOW_TO_ORIGIN(shadow) (((uptr)shadow) + 0x2000000000ULL)
+#define MEM_TO_ORIGIN(mem)       (SHADOW_TO_ORIGIN(MEM_TO_SHADOW(mem)))
+#define MEM_IS_APP(mem)          ((uptr)mem >= 0xe000000000ULL)
+#define MEM_IS_SHADOW(mem) \
+  ((uptr)mem >= 0xa000000000ULL && (uptr)mem <= 0xc000000000ULL)
+#elif defined(__x86_64__)
 #define MEM_TO_SHADOW(mem)       (((uptr)mem) & ~0x400000000000ULL)
 #define SHADOW_TO_ORIGIN(shadow) (((uptr)shadow) + 0x200000000000ULL)
 #define MEM_TO_ORIGIN(mem)       (SHADOW_TO_ORIGIN(MEM_TO_SHADOW(mem)))
 #define MEM_IS_APP(mem)          ((uptr)mem >= 0x600000000000ULL)
 #define MEM_IS_SHADOW(mem) \
   ((uptr)mem >= 0x200000000000ULL && (uptr)mem <= 0x400000000000ULL)
+#endif
 
 // These constants must be kept in sync with the ones in MemorySanitizer.cc.
 const int kMsanParamTlsSize = 800;
 const int kMsanRetvalTlsSize = 800;
 
 namespace __msan {
 extern int msan_inited;
 extern bool msan_init_is_running;

lib/msan/msan_allocator.cc

void OnUnmap(uptr p, uptr size) const {
     // We are about to unmap a chunk of user memory.
     // Mark the corresponding shadow memory as not needed.
     FlushUnneededShadowMemory(MEM_TO_SHADOW(p), size);
     if (__msan_get_track_origins())
       FlushUnneededShadowMemory(MEM_TO_ORIGIN(p), size);
   }
 };
 
+#if defined(__mips64)
+  static const uptr kMaxAllowedMallocSize = 2UL << 30;
+  static const uptr kRegionSizeLog = 20;
+  static const uptr kNumRegions = SANITIZER_MMAP_RANGE_SIZE >> kRegionSizeLog;
+  typedef TwoLevelByteMap<(kNumRegions >> 12), 1 << 12> ByteMap;
+  typedef CompactSizeClassMap SizeClassMap;
+
+  typedef SizeClassAllocator32<0, SANITIZER_MMAP_RANGE_SIZE, sizeof(Metadata),
+                               SizeClassMap, kRegionSizeLog, ByteMap,
+                               MsanMapUnmapCallback> PrimaryAllocator;
+#elif defined(__x86_64__)
   static const uptr kAllocatorSpace = 0x600000000000ULL;
   static const uptr kAllocatorSize   = 0x80000000000;  // 8T.
   static const uptr kMetadataSize  = sizeof(Metadata);
   static const uptr kMaxAllowedMallocSize = 8UL << 30;
 
   typedef SizeClassAllocator64<kAllocatorSpace, kAllocatorSize, kMetadataSize,
                              DefaultSizeClassMap,
                              MsanMapUnmapCallback> PrimaryAllocator;
+#endif
 typedef SizeClassAllocatorLocalCache<PrimaryAllocator> AllocatorCache;
 typedef LargeMmapAllocator<MsanMapUnmapCallback> SecondaryAllocator;
 typedef CombinedAllocator<PrimaryAllocator, AllocatorCache,
                           SecondaryAllocator> Allocator;
 
 static Allocator allocator;
 static AllocatorCache fallback_allocator_cache;
 static SpinMutex fallback_mutex;

lib/msan/msan_linux.cc

 #include <sys/time.h>
 #include <sys/resource.h>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_procmaps.h"
 
 namespace __msan {
 
+#if defined(__mips64)
+static const uptr kMemBeg     = 0xe000000000;
+static const uptr kMemEnd     = 0xffffffffff;
+#elif defined(__x86_64__)
 static const uptr kMemBeg     = 0x600000000000;
 static const uptr kMemEnd     = 0x7fffffffffff;
+#endif
+
 static const uptr kShadowBeg  = MEM_TO_SHADOW(kMemBeg);
 static const uptr kShadowEnd  = MEM_TO_SHADOW(kMemEnd);
 static const uptr kBad1Beg    = 0;
 static const uptr kBad1End    = kShadowBeg - 1;
 static const uptr kBad2Beg    = kShadowEnd + 1;
 static const uptr kBad2End    = kMemBeg - 1;
 static const uptr kOriginsBeg = kBad2Beg;
 static const uptr kOriginsEnd = kBad2End;

lib/msan/msan_report.cc

Printf(
       " in the stack frame of function '%s%s%s'%s\n",
       d.Origin(), d.Name(), s, d.Origin(), d.Name(), sep + 1, d.Origin(),
       d.End());
   InternalFree(s);
 
   if (pc) {
     // For some reason function address in LLVM IR is 1 less then the address
     // of the first instruction.
-    pc += 1;
+    pc = StackTrace::GetNextInstructionPc(pc);
     StackTrace(&pc, 1).Print();
   }
 }
 
 static void DescribeOrigin(u32 id) {
   VPrintf(1, "  raw origin id: %d\n", id);
   Decorator d;
   while (true) {
     Origin o(id);
     if (!o.isValid()) {

lib/sanitizer_common/sanitizer_platform.h

 #endif
 
 // The range of addresses which can be returned my mmap.
 // FIXME: this value should be different on different platforms,
 // e.g. on AArch64 it is most likely (1ULL << 39). Larger values will still work
 // but will consume more memory for TwoLevelByteMap.
 #if defined(__aarch64__)
 # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 39)
+#elif defined(__mips__)
+# define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 40)
 #else
 # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
 #endif
 
 // The AArch64 linux port uses the canonical syscall set as mandated by
 // the upstream linux community for all new ports. Other ports may still
 // use legacy syscalls.
 #ifndef SANITIZER_USES_CANONICAL_LINUX_SYSCALLS

lib/sanitizer_common/sanitizer_posix.cc

# if defined(__powerpc64__)
   // one of 0x00000fffffffffffUL and 0x00003fffffffffffUL.
   // Note that with 'ulimit -s unlimited' the stack is moved away from the top
   // of the address space, so simply checking the stack address is not enough.
   // This should (does) work for both PowerPC64 Endian modes.
   return (1ULL << (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1)) - 1;
 # elif defined(__aarch64__)
   return (1ULL << 39) - 1;
 # elif defined(__mips64)
-  return (1ULL << 40) - 1;
+  return (1ULL << 40) - 1;  // 0x000000ffffffffffUL;
 # else
   return (1ULL << 47) - 1;  // 0x00007fffffffffffUL;
 # endif
 #else  // SANITIZER_WORDSIZE == 32
   uptr res = (1ULL << 32) - 1;  // 0xffffffff;
   if (!common_flags()->full_address_space)
     res -= GetKernelAreaSize();
   CHECK_LT(reinterpret_cast<uptr>(&res), res);
   return res;

lib/sanitizer_common/sanitizer_stacktrace.h

if (!SANITIZER_CAN_FAST_UNWIND)
       return false;
     else if (!SANITIZER_CAN_SLOW_UNWIND)
       return true;
     return request_fast_unwind;
   }
 
   static uptr GetCurrentPc();
   static uptr GetPreviousInstructionPc(uptr pc);
+  static uptr GetNextInstructionPc(uptr pc);
   typedef bool (*SymbolizeCallback)(const void *pc, char *out_buffer,
                                     int out_size);
 };
 
 // StackTrace that owns the buffer used to store the addresses.
 struct BufferedStackTrace : public StackTrace {
   uptr trace_buffer[kStackTraceMax];
   uptr top_frame_bp;  // Optional bp of a top frame.

lib/sanitizer_common/sanitizer_stacktrace.cc

#if defined(__powerpc__) || defined(__powerpc64__)
   return pc - 4;
 #elif defined(__sparc__) || defined(__mips__)
   return pc - 8;
 #else
   return pc - 1;
 #endif
 }
 
+uptr StackTrace::GetNextInstructionPc(uptr pc) {
+#if defined(__mips__)
+  return pc + 8;
+#else
+  return pc + 1;
+#endif
+}
+
 uptr StackTrace::GetCurrentPc() {
   return GET_CALLER_PC();
 }
 
 void BufferedStackTrace::Init(const uptr *pcs, uptr cnt, uptr extra_top_pc) {
   size = cnt + !!extra_top_pc;
   CHECK_LE(size, kStackTraceMax);
   internal_memcpy(trace_buffer, pcs, cnt * sizeof(trace_buffer[0]));