[builtins] Avoid undefined behavior when calculating absolute value in floatsidf.c and floatsisf.c

[Ka-Ka]

Summary

When compiling compiler-rt with -fsanitize=undefined and running testcases you
end up with the following warning:

UBSan: floatsidf.c:32:9: negation of -2147483648 cannot be represented in type 'si_int' (aka 'long'); cast to an unsigned type to negate this value to itself

The same kind of pattern exists in floatsisf.c

This was found in an out of tree target.

[MaskRay]

compiler-rt/lib/builtins/floatsidf.c

33

Just use unary -: -(su_int)a

compiler-rt/lib/builtins/floatsisf.c

33

ditto

[MaskRay]

[compiler-rt]

[builtins] should be more specific. For many projects, we don't use the top-level project name as the tag (which can be simplified inferred from the toplevel path name, so less useful).

[Ka-Ka]

The patch was rebased and updated according to comments.

[MaskRay]

Sorry for just noticing the patch. https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted feel free to ping a patch when there is no response after a week :)

[MaskRay]

compiler-rt/lib/builtins/floatsidf.c

33

I think aAbs = -aAbs can be used as well, not sure which is clearer.
aAbs = -aAbs avoids a type conversion.

[Ka-Ka]

In D146123#4607489, @MaskRay wrote:

Sorry for just noticing the patch. https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted feel free to ping a patch when there is no response after a week :)

No problem. I didn't ping the review as this patch was not that important for us, as we in our of tree target have started to, as a workaround, compile compiler-rt with -fwrapv to avoid these kind of problems. I still think there are several signed integer overflow problem left to fix in the builtins in compiler-rt. If it is important to fix those I can probably take another look at this, as we in the future want to remove the use of -fwrapv.

[MaskRay]

In D146123#4609123, @Ka-Ka wrote:

In D146123#4607489, @MaskRay wrote:

Sorry for just noticing the patch. https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted feel free to ping a patch when there is no response after a week :)

No problem. I didn't ping the review as this patch was not that important for us, as we in our of tree target have started to, as a workaround, compile compiler-rt with -fwrapv to avoid these kind of problems. I still think there are several signed integer overflow problem left to fix in the builtins in compiler-rt. If it is important to fix those I can probably take another look at this, as we in the future want to remove the use of -fwrapv.

Sounds good! How do you build lib/builtins with -fsanitize=undefined?

compiler-rt provides runtime library of sanitizers and many components cannot be instrumented by sanitizers.

Building lib/builtins with non-trap-mode -fsanitize=undefined has a layering issue: sanitizer libraries depend on builtins, so builtins should not depend on sanitizer libraries.
Though with lld's archive semantics this violation is probably fine.

[Ka-Ka]

In D146123#4609136, @MaskRay wrote:

In D146123#4609123, @Ka-Ka wrote:

In D146123#4607489, @MaskRay wrote:

Sorry for just noticing the patch. https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted feel free to ping a patch when there is no response after a week :)

No problem. I didn't ping the review as this patch was not that important for us, as we in our of tree target have started to, as a workaround, compile compiler-rt with -fwrapv to avoid these kind of problems. I still think there are several signed integer overflow problem left to fix in the builtins in compiler-rt. If it is important to fix those I can probably take another look at this, as we in the future want to remove the use of -fwrapv.

Sounds good! How do you build lib/builtins with -fsanitize=undefined?

compiler-rt provides runtime library of sanitizers and many components cannot be instrumented by sanitizers.

Building lib/builtins with non-trap-mode -fsanitize=undefined has a layering issue: sanitizer libraries depend on builtins, so builtins should not depend on sanitizer libraries.
Though with lld's archive semantics this violation is probably fine.

I build in our out of tree target with -fsanitize=undefined and our own adapted -fsanitize-minimal-runtime.

compiler-rt/lib/builtins/floatsidf.c

COMPILER_RT_ABI fp_t __floatsidf(si_int a) {
   const int aWidth = sizeof a * CHAR_BIT;
 
   // Handle zero as a special case to protect clz
   if (a == 0)
     return fromRep(0);
 
   // All other cases begin by extracting the sign and absolute value of a
   rep_t sign = 0;
+  su_int aAbs = (su_int)a;
   if (a < 0) {
     sign = signBit;
-    a = -a;
+    aAbs = -(su_int)a;
   }
 
   // Exponent of (fp_t)a is the width of abs(a).
-  const int exponent = (aWidth - 1) - clzsi(a);
+  const int exponent = (aWidth - 1) - clzsi(aAbs);
   rep_t result;
 
   // Shift a into the significand field and clear the implicit bit.  Extra
   // cast to unsigned int is necessary to get the correct behavior for
   // the input INT_MIN.
   const int shift = significandBits - exponent;
-  result = (rep_t)(su_int)a << shift ^ implicitBit;
+  result = (rep_t)aAbs << shift ^ implicitBit;
 
   // Insert the exponent
   result += (rep_t)(exponent + exponentBias) << significandBits;
   // Insert the sign bit and return
   return fromRep(result | sign);
 }
 
 #if defined(__ARM_EABI__)
 #if defined(COMPILER_RT_ARMHF_TARGET)
 AEABI_RTABI fp_t __aeabi_i2d(si_int a) { return __floatsidf(a); }
 #else
 COMPILER_RT_ALIAS(__floatsidf, __aeabi_i2d)
 #endif
 #endif

compiler-rt/lib/builtins/floatsisf.c

COMPILER_RT_ABI fp_t __floatsisf(si_int a) {
   const int aWidth = sizeof a * CHAR_BIT;
 
   // Handle zero as a special case to protect clz
   if (a == 0)
     return fromRep(0);
 
   // All other cases begin by extracting the sign and absolute value of a
   rep_t sign = 0;
+  su_int aAbs = (su_int)a;
   if (a < 0) {
     sign = signBit;
-    a = -a;
+    aAbs = -(su_int)a;
   }
 
   // Exponent of (fp_t)a is the width of abs(a).
-  const int exponent = (aWidth - 1) - clzsi(a);
+  const int exponent = (aWidth - 1) - clzsi(aAbs);
   rep_t result;
 
   // Shift a into the significand field, rounding if it is a right-shift
   if (exponent <= significandBits) {
     const int shift = significandBits - exponent;
-    result = (rep_t)a << shift ^ implicitBit;
+    result = (rep_t)aAbs << shift ^ implicitBit;
   } else {
     const int shift = exponent - significandBits;
-    result = (rep_t)a >> shift ^ implicitBit;
-    rep_t round = (rep_t)a << (typeWidth - shift);
+    result = (rep_t)aAbs >> shift ^ implicitBit;
+    rep_t round = (rep_t)aAbs << (typeWidth - shift);
     if (round > signBit)
       result++;
     if (round == signBit)
       result += result & 1;
   }
 
   // Insert the exponent
   result += (rep_t)(exponent + exponentBias) << significandBits;

compiler-rt/lib/builtins/floatsitf.c

COMPILER_RT_ABI fp_t __floatsitf(si_int a) {
   if (a == 0)
     return fromRep(0);
 
   // All other cases begin by extracting the sign and absolute value of a
   rep_t sign = 0;
   su_int aAbs = (su_int)a;
   if (a < 0) {
     sign = signBit;
-    aAbs = ~(su_int)a + (su_int)1U;
+    aAbs = -(su_int)a;
   }
 
   // Exponent of (fp_t)a is the width of abs(a).
   const int exponent = (aWidth - 1) - clzsi(aAbs);
   rep_t result;
 
   // Shift a into the significand field and clear the implicit bit.
   const int shift = significandBits - exponent;