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

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

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

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
>