diff mbox

[LIBGCC] Avoid count_leading_zeros with undefined result (PR 78067)

Message ID AM4PR0701MB2162CCD9B18CE6257F940378E4AC0@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Oct. 29, 2016, 5:47 a.m. UTC 
On 10/28/16 16:05, Bernd Edlinger wrote:
> On 10/27/16 22:23, Joseph Myers wrote:

>> On Thu, 27 Oct 2016, Bernd Edlinger wrote:

>>

>>> Hi,

>>>

>>> by code reading I became aware that libgcc can call count_leading_zeros

>>> in certain cases which can give undefined results.  This happens on

>>> signed int128 -> float or double conversions, when the int128 is in

>>> the range

>>> INT64_MAX+1 to UINT64_MAX.

>>

>> I'd expect testcases added to the testsuite that exercise this case at

>> runtime, if not already present.

>>

>

> Yes, thanks.  I somehow expected there were already test cases,

> somewhere, but now when you ask that, I begin to doubt as well...

>

> I will try to add an asm("int 3") and see if that gets hit at all.

>


The breakpoint got hit only once, in the libgo testsuite: runtime/pprof.

I see there are some int to float conversion tests at
gcc.dg/torture/fp-int-convert*.c, where it is easy to add a
test case that hits the breakpoint too.

However the test case does not fail before the patch,
it is just slightly undefined behavior, that is not
causing problems (at least for x86_64).

Find attached a new patch with test case.


Boot-strapped on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

Comments

Bernd Schmidt Nov. 3, 2016, 12:15 p.m. UTC | #1 
On 10/29/2016 07:47 AM, Bernd Edlinger wrote:

> Find attached a new patch with test case.

>

>

> Boot-strapped on x86_64-pc-linux-gnu.

> Is it OK for trunk?


Ok.


Bernd
James Greenhalgh Nov. 9, 2016, 4:32 p.m. UTC | #2 
On Sat, Oct 29, 2016 at 05:47:54AM +0000, Bernd Edlinger wrote:
> On 10/28/16 16:05, Bernd Edlinger wrote:

> > On 10/27/16 22:23, Joseph Myers wrote:

> >> On Thu, 27 Oct 2016, Bernd Edlinger wrote:

> >>

> >>> Hi,

> >>>

> >>> by code reading I became aware that libgcc can call count_leading_zeros

> >>> in certain cases which can give undefined results.  This happens on

> >>> signed int128 -> float or double conversions, when the int128 is in

> >>> the range

> >>> INT64_MAX+1 to UINT64_MAX.

> >>

> >> I'd expect testcases added to the testsuite that exercise this case at

> >> runtime, if not already present.

> >>

> >

> > Yes, thanks.  I somehow expected there were already test cases,

> > somewhere, but now when you ask that, I begin to doubt as well...

> >

> > I will try to add an asm("int 3") and see if that gets hit at all.

> >

> 

> The breakpoint got hit only once, in the libgo testsuite: runtime/pprof.

> 

> I see there are some int to float conversion tests at

> gcc.dg/torture/fp-int-convert*.c, where it is easy to add a

> test case that hits the breakpoint too.

> 

> However the test case does not fail before the patch,

> it is just slightly undefined behavior, that is not

> causing problems (at least for x86_64).

> 

> Find attached a new patch with test case.


These new test cases look like they are going to be out of exponent range
for _Float16 - so the testcases will fail for a target which tests either
of:

  gcc.dg/torture/fp-int-convert-float16.c
  gcc.dg/torture/fp-int-convert-float16-timode.c

Should they have an M_OK1 check?

Thanks,
James


> 

> 

> Boot-strapped on x86_64-pc-linux-gnu.

> Is it OK for trunk?

> 

> 

> Thanks

> Bernd.


> 2016-10-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>

> 

> 	PR libgcc/78067

> 	* libgcc2.c (__floatdisf, __floatdidf): Avoid undefined results from

> 	count_leading_zeros.

> 

> testsuite:

> 2016-10-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>

> 

> 	PR libgcc/78067

> 	* gcc.dg/torture/fp-int-convert.h: Add more conversion tests.

> 	

> 

> Index: libgcc2.c

> ===================================================================

> --- libgcc2.c	(revision 241400)

> +++ libgcc2.c	(working copy)

> @@ -1643,6 +1643,11 @@

>      hi = -(UWtype) hi;

>  

>    UWtype count, shift;

> +#if !defined (COUNT_LEADING_ZEROS_0) || COUNT_LEADING_ZEROS_0 != W_TYPE_SIZE

> +  if (hi == 0)

> +    count = W_TYPE_SIZE;

> +  else

> +#endif

>    count_leading_zeros (count, hi);

>  

>    /* No leading bits means u == minimum.  */

> Index: gcc/testsuite/gcc.dg/torture/fp-int-convert.h

> ===================================================================

> --- gcc/testsuite/gcc.dg/torture/fp-int-convert.h	(revision 241647)

> +++ gcc/testsuite/gcc.dg/torture/fp-int-convert.h	(working copy)

> @@ -53,6 +53,8 @@ do {								\

>    TEST_I_F_VAL (U, F, HVAL1U (P, U), P_OK (P, U));		\

>    TEST_I_F_VAL (U, F, HVAL1U (P, U) + 1, P_OK (P, U));		\

>    TEST_I_F_VAL (U, F, HVAL1U (P, U) - 1, P_OK (P, U));		\

> +  TEST_I_F_VAL (I, F, WVAL0S (I), 1);				\

> +  TEST_I_F_VAL (I, F, -WVAL0S (I), 1);				\

>  } while (0)

>  

>  #define P_OK(P, T) ((P) >= sizeof(T) * CHAR_BIT)

> @@ -74,6 +76,7 @@ do {								\

>  			 ? (S)1						 \

>  			 : (((S)1 << (sizeof(S) * CHAR_BIT - 2))	 \

>  			    + ((S)3 << (sizeof(S) * CHAR_BIT - 2 - P))))

> +#define WVAL0S(S) (S)((S)1 << (sizeof(S) * CHAR_BIT / 2 - 1))

>  

>  #define TEST_I_F_VAL(IT, FT, VAL, PREC_OK)		\

>  do {							\
Joseph Myers Nov. 9, 2016, 4:52 p.m. UTC | #3 
On Wed, 9 Nov 2016, James Greenhalgh wrote:

> These new test cases look like they are going to be out of exponent range

> for _Float16 - so the testcases will fail for a target which tests either

> of:

> 

>   gcc.dg/torture/fp-int-convert-float16.c

>   gcc.dg/torture/fp-int-convert-float16-timode.c

> 

> Should they have an M_OK1 check?


Yes.

-- 
Joseph S. Myers
joseph@codesourcery.com
Bernd Edlinger Nov. 9, 2016, 5:31 p.m. UTC | #4 
On 11/09/16 17:52, Joseph Myers wrote:
> On Wed, 9 Nov 2016, James Greenhalgh wrote:

>

>> These new test cases look like they are going to be out of exponent range

>> for _Float16 - so the testcases will fail for a target which tests either

>> of:

>>

>>   gcc.dg/torture/fp-int-convert-float16.c

>>   gcc.dg/torture/fp-int-convert-float16-timode.c

>>

>> Should they have an M_OK1 check?

>

> Yes.

>


Yes, but maybe introduce a test if the half-wide value fits?

like:

#define M_OK2(M, T) ((M) > sizeof(T) * CHAR_BIT / 2 - 1)


Bernd.
Joseph Myers Nov. 9, 2016, 10:16 p.m. UTC | #5 
On Wed, 9 Nov 2016, Bernd Edlinger wrote:

> Yes, but maybe introduce a test if the half-wide value fits?

> 

> like:

> 

> #define M_OK2(M, T) ((M) > sizeof(T) * CHAR_BIT / 2 - 1)


Something like that.

-- 
Joseph S. Myers
joseph@codesourcery.com
diff mbox

Patch

2016-10-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR libgcc/78067
	* libgcc2.c (__floatdisf, __floatdidf): Avoid undefined results from
	count_leading_zeros.

testsuite:
2016-10-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR libgcc/78067
	* gcc.dg/torture/fp-int-convert.h: Add more conversion tests.
	

Index: libgcc2.c
===================================================================
--- libgcc2.c	(revision 241400)
+++ libgcc2.c	(working copy)
@@ -1643,6 +1643,11 @@ 
     hi = -(UWtype) hi;
 
   UWtype count, shift;
+#if !defined (COUNT_LEADING_ZEROS_0) || COUNT_LEADING_ZEROS_0 != W_TYPE_SIZE
+  if (hi == 0)
+    count = W_TYPE_SIZE;
+  else
+#endif
   count_leading_zeros (count, hi);
 
   /* No leading bits means u == minimum.  */
Index: gcc/testsuite/gcc.dg/torture/fp-int-convert.h
===================================================================
--- gcc/testsuite/gcc.dg/torture/fp-int-convert.h	(revision 241647)
+++ gcc/testsuite/gcc.dg/torture/fp-int-convert.h	(working copy)
@@ -53,6 +53,8 @@  do {								\
   TEST_I_F_VAL (U, F, HVAL1U (P, U), P_OK (P, U));		\
   TEST_I_F_VAL (U, F, HVAL1U (P, U) + 1, P_OK (P, U));		\
   TEST_I_F_VAL (U, F, HVAL1U (P, U) - 1, P_OK (P, U));		\
+  TEST_I_F_VAL (I, F, WVAL0S (I), 1);				\
+  TEST_I_F_VAL (I, F, -WVAL0S (I), 1);				\
 } while (0)
 
 #define P_OK(P, T) ((P) >= sizeof(T) * CHAR_BIT)
@@ -74,6 +76,7 @@  do {								\
 			 ? (S)1						 \
 			 : (((S)1 << (sizeof(S) * CHAR_BIT - 2))	 \
 			    + ((S)3 << (sizeof(S) * CHAR_BIT - 2 - P))))
+#define WVAL0S(S) (S)((S)1 << (sizeof(S) * CHAR_BIT / 2 - 1))
 
 #define TEST_I_F_VAL(IT, FT, VAL, PREC_OK)		\
 do {							\