| Message ID | HE1PR0701MB21698B2BB65F953F78D09E02E4AA0@HE1PR0701MB2169.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 10/27/2016 01:27 PM, 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. Did you actually observe this? Because, prior to this code, > 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. */ we have: /* If there are no high bits set, fall back to one conversion. */ if ((Wtype)u == u) return (FSTYPE)(Wtype)u; /* Otherwise, find the power of two. */ Wtype hi = u >> W_TYPE_SIZE; which looks to me like it ensures that hi is != 0. Bernd
On 10/27/16 14:52, Bernd Schmidt wrote: > On 10/27/2016 01:27 PM, 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. > > Did you actually observe this? Because, prior to this code, > >> 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. */ > > we have: > > /* If there are no high bits set, fall back to one conversion. */ > if ((Wtype)u == u) > return (FSTYPE)(Wtype)u; > > /* Otherwise, find the power of two. */ > Wtype hi = u >> W_TYPE_SIZE; > > which looks to me like it ensures that hi is != 0. > No. I have been there in the debugger. But with Wtype = long long and thus (long long)(2^64-1) = -1LL and the if is not taken, as (__int128)-1LL != (__int128)(2^64-1). but 2^64-1 >> 64 = 0. Now it the BSR instruction is used, with in=out=si, it is not specified, what will be the result in this case, on my machine AMD64-someting, it left the si register untouched. and the function continued, as if count was 63 in that case, but to be correct, it should be 64. In the function below we have if ((UWtype)u == u) that actually ensures hi != 0. Bernd.
On 10/27/2016 05:57 PM, Bernd Edlinger wrote: > In the function below we have if ((UWtype)u == u) > that actually ensures hi != 0. Ah, right. So maybe we ought to just add the same case here as well? if ((UWtype)u == u) return (FSTYPE)(UWtype)u; That would also make the comment less misleading. The condition should ensure that u is positive and representable in UWtype, so this should be correct, right? Bernd
On 10/27/16 20:04, Bernd Schmidt wrote: > On 10/27/2016 05:57 PM, Bernd Edlinger wrote: >> In the function below we have if ((UWtype)u == u) >> that actually ensures hi != 0. > > Ah, right. So maybe we ought to just add the same case here as well? > > if ((UWtype)u == u) > return (FSTYPE)(UWtype)u; > > That would also make the comment less misleading. The condition should > ensure that u is positive and representable in UWtype, so this should be > correct, right? > you mean: if ((Wtype)u == u) return (FSTYPE)(Wtype)u; if ((UWtype)u == u) return (FSTYPE)(UWtype)u; I think, that should work as well, yes. OTOH there is a possibility that COUNT_LEADING_ZEROS_0 indicates that we will not need any extra checks here. Bernd.
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. -- Joseph S. Myers joseph@codesourcery.com
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. Bernd.
2016-10-27 Bernd Edlinger <bernd.edlinger@hotmail.de> PR libgcc/78067 * libgcc2.c (__floatdisf, __floatdidf): Avoid undefined results from count_leading_zeros. 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. */