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

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
>

> On Oct 23, 2015, at 7:43 PM, Matthew Fernandez via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 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.
FWIW, another way to avoid the UB is to use an unsigned value.

-Chris

On Oct 24, 2015, at 6:02 PM, Chris Lattner <clattner at apple.com>
wrote:
> 
> 
>> On Oct 23, 2015, at 7:43 PM, Matthew Fernandez via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>> 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.
> 
> FWIW, another way to avoid the UB is to use an unsigned value.
I'm confused, that's exactly what this does.

- Steve