[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

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 :wink:

FWIW, another way to avoid the UB is to use an unsigned value.

-Chris

I'm confused, that's exactly what this does.

- Steve

At least in one place it is negating the signed variable, not creating a
new unsigned variable.

Joerg

Chris, were you suggesting a cast (`a = -(unsigned)a`)? Otherwise, like Steve, I don't understand the difference between this proposal and Steve's code above.

I started an attempt at this today, but the current revision, r251669,
doesn't seem to build for me. I don't recall having problems building
Clang in the past and this time I just followed the standard getting
started instructions [0] without any odd flags in my environment. The
actual error is pages long, but is essentially a link failure when
forming libc++.so caused by failing to find vtables for things like
`__cxxabiv1::__si_class_type_info`. Is this a known issue? If the
solution is not straightforward, perhaps I should move this over to
Bugzilla.

  [0]: http://clang.llvm.org/get_started.html

... 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've posted a version of (1) to llvm-commits and CCed the participants of this thread. If all goes well, I'll send another patch for (3).