diff mbox series

libgcc: Fix UB in FP_FROM_BITINT

Message ID ZcsjU8IKKTZ6pWIE@tucnak
State New
Headers show
Series libgcc: Fix UB in FP_FROM_BITINT | expand

Commit Message

Jakub Jelinek Feb. 13, 2024, 8:07 a.m. UTC 
Hi!

As I wrote earlier, I was seeing
FAIL: gcc.dg/torture/bitint-24.c   -O0  execution test
FAIL: gcc.dg/torture/bitint-24.c   -O2  execution test
with the ia32 _BitInt enablement patch on i686-linux.  I thought
floatbitintxf.c was miscompiled with -O2 -march=i686 -mtune=generic, but it
turned out to be UB in it.

If a signed _BitInt to be converted to binary floating point has
(after sign extension from possible partial limb to full limb) one or
more most significant limbs equal to all ones and then in the limb below
(the most significant non-~(UBILtype)0 limb) has the most significant limb
cleared, like for 32-bit limbs
0x81582c05U, 0x0a8b01e4U, 0xc1b8b18fU, 0x2aac2a08U, -1U, -1U
then bitint_reduce_prec can't reduce it to that 0x2aac2a08U limb, so
msb is all ones and precision is negative (so it reduced precision from
161 to 192 bits down to 160 bits, in theory could go as low as 129 bits
but that wouldn't change anything on the following behavior).
But still iprec is negative, -160 here.
For that case (i.e. where we are dealing with an negative input), the
code was using 65 - __builtin_clzll (~msb) to compute how many relevant
bits we have from the msb.  Unfortunately that invokes UB for msb all ones.
The right number of relevant bits in that case is 1 though (like for
-2 it is 2 and -4 or -3 3 as already computed) - all we care about from that
is that the most significant bit is set (i.e. the number is negative) and
the bits below that should be supplied from the limbs below.

So, the following patch fixes it by special casing it not to invoke UB.

For msb 0 we already have a special case from before (but that is also
different because msb 0 implies the whole number is 0 given the way
bitint_reduce_prec works - even if we have limbs like ..., 0x80000000U, 0U
the reduction can skip the most significant limb and msb then would be
the one below it), so if iprec > 0, we already don't call __builtin_clzll
on 0.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-02-13  Jakub Jelinek  <jakub@redhat.com>

	* soft-fp/bitint.h (FP_FROM_BITINT): If iprec < 0 and msb is all ones,
	just set n to 1 instead of using __builtin_clzll (~msb).



	Jakub

Comments

Richard Biener Feb. 13, 2024, 9:09 a.m. UTC | #1 
On Tue, 13 Feb 2024, Jakub Jelinek wrote:

> Hi!
> 
> As I wrote earlier, I was seeing
> FAIL: gcc.dg/torture/bitint-24.c   -O0  execution test
> FAIL: gcc.dg/torture/bitint-24.c   -O2  execution test
> with the ia32 _BitInt enablement patch on i686-linux.  I thought
> floatbitintxf.c was miscompiled with -O2 -march=i686 -mtune=generic, but it
> turned out to be UB in it.
> 
> If a signed _BitInt to be converted to binary floating point has
> (after sign extension from possible partial limb to full limb) one or
> more most significant limbs equal to all ones and then in the limb below
> (the most significant non-~(UBILtype)0 limb) has the most significant limb
> cleared, like for 32-bit limbs
> 0x81582c05U, 0x0a8b01e4U, 0xc1b8b18fU, 0x2aac2a08U, -1U, -1U
> then bitint_reduce_prec can't reduce it to that 0x2aac2a08U limb, so
> msb is all ones and precision is negative (so it reduced precision from
> 161 to 192 bits down to 160 bits, in theory could go as low as 129 bits
> but that wouldn't change anything on the following behavior).
> But still iprec is negative, -160 here.
> For that case (i.e. where we are dealing with an negative input), the
> code was using 65 - __builtin_clzll (~msb) to compute how many relevant
> bits we have from the msb.  Unfortunately that invokes UB for msb all ones.
> The right number of relevant bits in that case is 1 though (like for
> -2 it is 2 and -4 or -3 3 as already computed) - all we care about from that
> is that the most significant bit is set (i.e. the number is negative) and
> the bits below that should be supplied from the limbs below.
> 
> So, the following patch fixes it by special casing it not to invoke UB.
> 
> For msb 0 we already have a special case from before (but that is also
> different because msb 0 implies the whole number is 0 given the way
> bitint_reduce_prec works - even if we have limbs like ..., 0x80000000U, 0U
> the reduction can skip the most significant limb and msb then would be
> the one below it), so if iprec > 0, we already don't call __builtin_clzll
> on 0.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> 2024-02-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* soft-fp/bitint.h (FP_FROM_BITINT): If iprec < 0 and msb is all ones,
> 	just set n to 1 instead of using __builtin_clzll (~msb).
> 
> --- libgcc/soft-fp/bitint.h.jj	2024-01-12 22:06:06.248661862 +0100
> +++ libgcc/soft-fp/bitint.h	2024-02-12 18:56:42.947974875 +0100
> @@ -276,7 +276,11 @@ bitint_negate (UBILtype *d, const UBILty
>  	}								\
>        if (iprec < 0)							\
>  	{								\
> -	  n = sizeof (0ULL) * __CHAR_BIT__ + 1 - __builtin_clzll (~msb);\
> +	  if (msb == (UBILtype) -1)					\
> +	    n = 1;							\
> +	  else								\
> +	    n = (sizeof (0ULL) * __CHAR_BIT__ + 1			\
> +		 - __builtin_clzll (~msb));				\
>  	  if (BIL_TYPE_SIZE > DI##_BITS && n > DI##_BITS)		\
>  	    {								\
>  	      iv = msb >> (n - DI##_BITS);				\
> 
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- libgcc/soft-fp/bitint.h.jj	2024-01-12 22:06:06.248661862 +0100
+++ libgcc/soft-fp/bitint.h	2024-02-12 18:56:42.947974875 +0100
@@ -276,7 +276,11 @@  bitint_negate (UBILtype *d, const UBILty
 	}								\
       if (iprec < 0)							\
 	{								\
-	  n = sizeof (0ULL) * __CHAR_BIT__ + 1 - __builtin_clzll (~msb);\
+	  if (msb == (UBILtype) -1)					\
+	    n = 1;							\
+	  else								\
+	    n = (sizeof (0ULL) * __CHAR_BIT__ + 1			\
+		 - __builtin_clzll (~msb));				\
 	  if (BIL_TYPE_SIZE > DI##_BITS && n > DI##_BITS)		\
 	    {								\
 	      iv = msb >> (n - DI##_BITS);				\