[llvm-dev] [compiler-rt] Undefined negation in float emulation functions

[Matthew Fernandez via llvm-dev]

Thanks for the confirmation, Steve. Your suggestion looks good to me, but I don't have an environment set up to build 
the test suite so it may take me a little while to get back to you with a validated patch.

A bit of creative grepping yields the following that also look problematic to me:

     compiler-rt/test/builtins/Unit/absvsi2_test.c:         expected = -expected;
     compiler-rt/test/builtins/Unit/absvti2_test.c:         expected = -expected;
     compiler-rt/test/builtins/Unit/absvdi2_test.c:         expected = -expected;

I confess I don't fully understand the way these tests are written. E.g. compiler-rt/test/builtins/Unit/absvsi2_test.c:

     int test__absvsi2(si_int a)
     {
         si_int x = __absvsi2(a);
         si_int expected = a;
         if (expected < 0)
             expected = -expected;
         if (x != expected || expected < 0)
             printf("error in __absvsi2(0x%X) = %d, expected positive %d\n",
                    a, x, expected);
         return x != expected;
     }

The printf suggests to me that the test should fail if `expected` is negative (which seems perfectly reasonable), but 
the return statement does not include this condition. I tried to explore this a little, but it turns out calling 
__absvisi2 with INT_MIN triggers compilerrt_abort(). Seems perfectly reasonable as abs(INT_MIN) is documented to be 
undefined, however the call to compilerrt_abort sits behind this check:

     const int N = (int)(sizeof(si_int) * CHAR_BIT);
     if (a == (1 << (N-1)))
         compilerrt_abort();

The shift in this expression is done on a signed int (the literal "1") and I believe shifting into the sign bit like 
this is also UB.

Where do you suggest we go from here? My intended approach is (1) propose a patch that avoids the signed negation in 
__floatsidf and __floatsisf as you suggested, (2) leave test__absv.i2 as-is as INT_MIN as an input is documented to be 
undefined, and (3) propose a patch that rephrases the left shifts in __absv.i2 to avoid UB there. I don't want to cause 
unnecessary headaches, so if there is a better way to go about this or if you disagree with anything I've said please 
let me know. And to think this was just supposed to be a quick afternoon tinkering with LLVM for me ;)

On 21/10/15 00:15, Stephen Canon wrote:
> Yup, this is UB.  If you want to propose a patch, I would do something like the following:
>
> 	rep_t sign = 0;
> 	unsigned int aAbs = a;
> 	if (a < 0) {
> 		sign = signBit;
> 		aAbs = -aAbs;
> 	}
> 	// Now use aAbs instead of a.
>
> – Steve
>
>> On Oct 20, 2015, at 6:38 AM, Matthew Fernandez via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>>
>> Hi,
>>
>> I recently came across the following in __floatsidf in compiler-rt:
>>
>>     __floatsidf(int a) {
>>         ...
>>         if (a < 0) {
>>             ...
>>             a = -a;
>>
>> In the case where a == INT_MIN, is this negation not undefined behaviour? AIUI this function is used for software emulation on targets that have no hardware floating point support. Perhaps there is an in-built assumption that this code is never called with INT_MIN, though I don't immediately see anything to indicate this. Indeed there is a later comment in this function that indicates INT_MIN is an anticipated input, but the negation has already occurred by this point.
>>
>> I am not a floating point expert, so perhaps I am missing some subtlety here. If so, apologies for the noise. The above refers to r218935 and similar code is present in __floatsisf.
>>
>> Thanks,
>> Matthew
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>