[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

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

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

Just use Opts.pc instead of Loc here.

273

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

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

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

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).

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

lib/sanitizer_common/sanitizer_stacktrace.cc

 namespace __sanitizer {
 
 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();

lib/ubsan/ubsan_handlers.cc

 //===-- ubsan_handlers.cc -------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Error logging entry points for the UBSan runtime.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ubsan_handlers.h"
 #include "ubsan_diag.h"
-
+#include "sanitizer_common/sanitizer_stacktrace.h"
 #include "sanitizer_common/sanitizer_common.h"
 
 using namespace __sanitizer;
 using namespace __ubsan;
 
 namespace __ubsan {
   const char *TypeCheckKinds[] = {
     "load of", "store to", "reference binding to", "member access within",
void __ubsan::__ubsan_handle_vla_bound_not_positive(VLABoundData *Data,
   handleVLABoundNotPositive(Data, Bound, Opts);
 }
 void __ubsan::__ubsan_handle_vla_bound_not_positive_abort(VLABoundData *Data,
                                                           ValueHandle Bound) {
   GET_REPORT_OPTIONS(true);
   handleVLABoundNotPositive(Data, Bound, Opts);
 }
 
-static void handleFloatCastOverflow(FloatCastOverflowData *Data,
+static void handleFloatCastOverflow(FloatCastOverflowData *Data, uptr Loc,
                                     ValueHandle From, ReportOptions Opts) {
   // TODO: Add deduplication once a SourceLocation is generated for this check.
   ScopedReport R(Opts);
 
-  Diag(getCallerLocation(), DL_Error,
+  Diag(getCallerLocation(Loc), DL_Error,
        "value %0 is outside the range of representable values of type %2")
     << Value(Data->FromType, From) << Data->FromType << Data->ToType;
 }
 
 void __ubsan::__ubsan_handle_float_cast_overflow(FloatCastOverflowData *Data,
                                                  ValueHandle From) {
   GET_REPORT_OPTIONS(false);
-  handleFloatCastOverflow(Data, From, Opts);
+  uptr Loc=GET_CALLER_PC();
+  handleFloatCastOverflow(Data, Loc, From, Opts);
 }
 void
 __ubsan::__ubsan_handle_float_cast_overflow_abort(FloatCastOverflowData *Data,
                                                   ValueHandle From) {
   GET_REPORT_OPTIONS(true);
-  handleFloatCastOverflow(Data, From, Opts);
+  uptr Loc=GET_CALLER_PC();
+  handleFloatCastOverflow(Data, Loc, From, Opts);
 }
 
 static void handleLoadInvalidValue(InvalidValueData *Data, ValueHandle Val,
                                    ReportOptions Opts) {
   SourceLocation Loc = Data->Loc.acquire();
   if (Loc.isDisabled())
     return;
 

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

 // 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 <stdint.h>
 #include <stdio.h>
 #include <string.h>
+#include <endian.h>
 
 float Inf;
 float NaN;
 
 int main(int argc, char **argv) {
   float MaxFloatRepresentableAsInt = 0x7fffff80;
   (int)MaxFloatRepresentableAsInt; // ok
   (int)-MaxFloatRepresentableAsInt; // ok
 
   float MinFloatRepresentableAsInt = -0x7fffffff - 1;
   (int)MinFloatRepresentableAsInt; // ok
 
   float MaxFloatRepresentableAsUInt = 0xffffff00u;
   (unsigned int)MaxFloatRepresentableAsUInt; // ok
 
 #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, 0x10 };
+#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.