[UBSan] Adding support of MIPS32

[slthakur]

Summary

Changed files:
config-ix.cmake: Enabled UBSan for MIPS32
sanitizer_stacktrace.cc: Program counter for MIPS32 is four byte aligned and a delay slot so subtracted PC by 8 for getting call site address.
cast-overflow.cpp: Added big endian support for this test case.

[petarj]

lib/sanitizer_common/sanitizer_stacktrace.cc

26 ↗

(On Diff #12441)

While not the subject of this code review, I have to say that the name of this function seems to be wrong. (otherwise, it needs some explanation in a form of a code comment)
From what I read in the rest of the code, this is used to convert a return-address value into a call-site address, right?
If that is correct, I am puzzled with the ARM part which does nothing for non-thumb addresses.
Further, (if my understanding is still correct,) MIPS should follow SPARC case for MIPS32r1/r2, as there is a delay slot to be taken into account.
Last, just a comment that this code is likely to become more complex with compact branches in MIPS32r6, microMIPS, etc.

[kcc]

LGTM

[slthakur]

Changed files:
sanitizer_stacktrace.cc: Considering the delay slot in MIPS32 PC should be subtracted by 8.
ubsan_handlers.cc: __ubsan_handle_float_cast_overflow should give callers PC to handleFloatCastOverflow to print the correct location. Else it prints the location of call site to be in ubsan_handlers.cc itself.
cast-overflow.cpp: Added support for big endian machines to cast-overflow test case.

[kcc]

lib/ubsan/ubsan_handlers.cc

273 ↗

(On Diff #13198)

please add spaces around '='

[kcc]

Also, I'd prefer samsonov@ to review ubsan-related changes. (Somehow I can't CC him here...)

[samsonov]

lib/ubsan/ubsan_handlers.cc

265 ↗

(On Diff #13198)

Just use Opts.pc instead of Loc here.

273 ↗

(On Diff #13198)

You don't need this line, caller's PC is already collected in GET_REPORT_OPTIONS() macro.

[slthakur]

Updated patch as per suggestion by samsonov.

[petarj]

test/ubsan/TestCases/Float/cast-overflow.cpp

57 ↗

(On Diff #14036)

Why is this value different than the one for little endian?

[samsonov]

LGTM. Thanks!

Note that we don't have a MIPS bot, so we can't guarantee we won't accidentally break this arch.

[slthakur]

Updated patch as there was a mistake in the value of NaN for big endian.

[samsonov]

LGTM

[petarj]

LGTM

[petarj]

test/ubsan/TestCases/Float/cast-overflow.cpp

20 ↗

(On Diff #14064)

nit: put this #include before #include <stdint.h>

[slthakur]

Moved #include <endian.h> before #include <stdint.h>

[petarj]

Great. Do you need someone to commit this for you?

[dsanders]

Note that we don't have a MIPS bot, so we can't guarantee we won't accidentally break this arch.

I'm in the process of sorting out some kit for new buildbots. I'll add a sanitizer buildslave to the todo list.

[slthakur]

This comment was removed by slthakur.

lib/sanitizer_common/sanitizer_stacktrace.cc

26 ↗

(On Diff #12441)

I had not considered the delay slot. I have updated it in the new revision.

[slthakur]

In D4881#36, @petarj wrote:

Great. Do you need someone to commit this for you?

Yes, I don't have commit access.

[petarj]

Closed by commit rL218519 (authored by @petarj).

compiler-rt/trunk/cmake/config-ix.cmake

 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(PROFILE_SUPPORTED_ARCH x86_64 i386 arm aarch64)
 filter_available_targets(TSAN_SUPPORTED_ARCH x86_64)
-filter_available_targets(UBSAN_SUPPORTED_ARCH x86_64 i386 arm aarch64)
+filter_available_targets(UBSAN_SUPPORTED_ARCH x86_64 i386 arm aarch64 mips)
 
 if(ANDROID)
   set(OS_NAME "Android")
 else()
   set(OS_NAME "${CMAKE_SYSTEM_NAME}")
 endif()
 
 if (SANITIZER_COMMON_SUPPORTED_ARCH AND NOT LLVM_USE_SANITIZER AND

compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace.cc

 uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
 #if defined(__arm__)
   // Cancel Thumb bit.
   pc = pc & (~1);
 #endif
 #if defined(__powerpc__) || defined(__powerpc64__)
   // PCs are always 4 byte aligned.
   return pc - 4;
-#elif defined(__sparc__)
+#elif defined(__sparc__) || defined(__mips__)
   return pc - 8;
 #else
   return pc - 1;
 #endif
 }
 
 uptr StackTrace::GetCurrentPc() {
   return GET_CALLER_PC();

compiler-rt/trunk/test/ubsan/TestCases/Float/cast-overflow.cpp

 // FIXME: run this (and other) UBSan tests in both 32- and 64-bit modes (?).
 // RUN: %clangxx -fsanitize=float-cast-overflow %s -o %t
 // RUN: %run %t _
 // RUN: %run %t 0 2>&1 | FileCheck %s --check-prefix=CHECK-0
 // RUN: %run %t 1 2>&1 | FileCheck %s --check-prefix=CHECK-1
 // RUN: %run %t 2 2>&1 | FileCheck %s --check-prefix=CHECK-2
 // RUN: %run %t 3 2>&1 | FileCheck %s --check-prefix=CHECK-3
 // RUN: %run %t 4 2>&1 | FileCheck %s --check-prefix=CHECK-4
 // RUN: %run %t 5 2>&1 | FileCheck %s --check-prefix=CHECK-5
 // RUN: %run %t 6 2>&1 | FileCheck %s --check-prefix=CHECK-6
 // FIXME: %run %t 7 2>&1 | FileCheck %s --check-prefix=CHECK-7
 // RUN: not %run %t 8 2>&1 | FileCheck %s --check-prefix=CHECK-8
 // RUN: not %run %t 9 2>&1 | FileCheck %s --check-prefix=CHECK-9
 
 // This test assumes float and double are IEEE-754 single- and double-precision.
 
+#include <endian.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <string.h>
 
 float Inf;
 float NaN;
 
 int main(int argc, char **argv) {
#ifdef __SIZEOF_INT128__
   unsigned __int128 FloatMaxAsUInt128 = -((unsigned __int128)1 << 104);
   (void)(float)FloatMaxAsUInt128; // ok
 #endif
 
   float NearlyMinusOne = -0.99999;
   unsigned Zero = NearlyMinusOne; // ok
 
   // Build a '+Inf'.
+#if __BYTE_ORDER == __LITTLE_ENDIAN
   char InfVal[] = { 0x00, 0x00, 0x80, 0x7f };
+#else
+  char InfVal[] = { 0x7f, 0x80, 0x00, 0x00 };
+#endif
   float Inf;
   memcpy(&Inf, InfVal, 4);
 
   // Build a 'NaN'.
+#if __BYTE_ORDER == __LITTLE_ENDIAN
   char NaNVal[] = { 0x01, 0x00, 0x80, 0x7f };
+#else
+  char NaNVal[] = { 0x7f, 0x80, 0x00, 0x01 };
+#endif
   float NaN;
   memcpy(&NaN, NaNVal, 4);
 
   double DblInf = (double)Inf; // ok
 
   switch (argv[1][0]) {
     // FIXME: Produce a source location for these checks and test for it here.