[UBSan][MIPS] Use unsigned value while bit shifting on MIPS

[slthakur]

Summary

On MIPS, shift operation on signed types give unpredictable results. Therefore we use unsigned value while shifting and after shifting is done we convert it back to signed value.

[samsonov]

Please elaborate what's going on here (i.e. what is the value that we fail to sign-extend)? This code looks far too complex to me, and I'd strongly suggest to make it platform-independent.

[slthakur]

Please elaborate what's going on here (i.e. what is the value that we fail to sign-extend)?

The value that we fail to sign extend is of SIntMax(Val). We are getting uncertain results on bit shifting of signed types. Therefore we need to use unsigned types when applying bit shifting. Since return type of getSIntValue() function is SIntMax, we can type cast the return value after bit shifting to SIntMax.

This code looks far too complex to me, and I'd strongly suggest to make it platform-independent.

I have tried to simplify the code and make it platform independent. I will update this patch with the simplified code

[slthakur]

Simplified code and made it platform-independent.

[dsanders]

Could you confirm the type of SIntMax and the value of ExtraBits when it gives incorrect results? 'X << N >> N' is a common idiom for sign-extension so it's surprising that it's not working for you.

[slthakur]

The type of SIntMax is __int128. The value of ExtraBits is 64 when getType().getIntegerBitWidth() returns 64 and it is 96 when getType().getIntegerBitWidth() returns 32.

[dsanders]

I haven't identified the guilty instruction(s) yet but it looks like a CodeGen bug with 128-bit shifts. At the moment '(x << 64) == x' and '(x << 32) == (x << 96)' neither of which should be true.

[samsonov]

What Daniel says. Looks like you should fix the MIPS CodeGen instead. As a workaround, you can try to hack ubsan_value.h to treat __int128 as unavailable on MIPS.

[slthakur]

http://reviews.llvm.org/D9337 fixes 128 bit shifts for mips in compiler.

lib/ubsan/ubsan_value.cc

 //===-- ubsan_value.cc ----------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Representation of a runtime value, as marshaled from the generated code to
 // the ubsan runtime.
 //
 //===----------------------------------------------------------------------===//
 
+#include <stdlib.h>
 #include "ubsan_platform.h"
 #if CAN_SANITIZE_UB
 #include "ubsan_value.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_libc.h"
 
 using namespace __ubsan;
 
 SIntMax Value::getSIntValue() const {
   CHECK(getType().isSignedIntegerTy());
   if (isInlineInt()) {
     // Val was zero-extended to ValueHandle. Sign-extend from original width
     // to SIntMax.
     const unsigned ExtraBits =
       sizeof(SIntMax) * 8 - getType().getIntegerBitWidth();
-    return SIntMax(Val) << ExtraBits >> ExtraBits;
+    UIntMax Value;
+    if(getType().getIntegerBitWidth() == 32)
+      Value = UIntMax(abs(Val));
+    else
+      Value = UIntMax(llabs(Val));
+    Value = Value << ExtraBits >> ExtraBits;
+    uptr Mask = 1UL << (getType().getIntegerBitWidth() - 1);
+    if(Mask & Val)
+      return -(SIntMax(Value));
+    return SIntMax(Value);
   }
   if (getType().getIntegerBitWidth() == 64)
     return *reinterpret_cast<s64*>(Val);
 #if HAVE_INT128_T
   if (getType().getIntegerBitWidth() == 128)
     return *reinterpret_cast<s128*>(Val);
 #else
   if (getType().getIntegerBitWidth() == 128)