[PATCH] fix llrint and lrint for 52 <= exponent <= 62

Discussion:

Matthias J. Kannwischer

2018-05-14 13:38:33 UTC

lrint and llrint produce wrong results if the exponent is between 52 and 62.
e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048.
Can you confirm this?
I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below.

Cheers,
Matthias

---
newlib/libm/common/s_llrint.c | 4 ++--
newlib/libm/common/s_lrint.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c
index 8b8a846ae..72452dbe9 100644
--- a/newlib/libm/common/s_llrint.c
+++ b/newlib/libm/common/s_llrint.c
@@ -93,9 +93,9 @@ long long int
if (j0 >= 52)
/* 64bit return: j0 in [52,62] */
/* 64bit return: left shift amt in [32,42] */
- result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) |
+ result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
/* 64bit return: right shift amt in [0,10] */
- (i1 << (j0 - 52));
+ ((long long int) i1 << (j0 - 52));
else
{
/* 64bit return: j0 in [20,51] */
diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c
index 9d2cb7306..b37f50fd4 100644
--- a/newlib/libm/common/s_lrint.c
+++ b/newlib/libm/common/s_lrint.c
@@ -131,9 +131,9 @@ TWO52[2]={
if (j0 >= 52)
/* 64bit return: j0 in [52,62] */
/* 64bit return: left shift amt in [32,42] */
- result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) |
+ result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
/* 64bit return: right shift amt in [0,10] */
- (i1 << (j0 - 52));
+ ((long int) i1 << (j0 - 52));
else
{
/* 32bit return: j0 in [20,30] */

--
2.14.3

Corinna Vinschen

2018-05-29 13:25:21 UTC

Permalink

Hi Matthias,

Post by Matthias J. Kannwischer
lrint and llrint produce wrong results if the exponent is between 52 and 62.
e.g. llrint(2.308393893084546048E18) returns 146666059061806080 instead of 2308393893084546048.
Can you confirm this?
I've tracked this down to two mistakes in s_llrint.c/s_lrint.c (one missing 0 and one uint32 overflow), see patch below.
Cheers,
Matthias
---
newlib/libm/common/s_llrint.c | 4 ++--
newlib/libm/common/s_lrint.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/newlib/libm/common/s_llrint.c b/newlib/libm/common/s_llrint.c
index 8b8a846ae..72452dbe9 100644
--- a/newlib/libm/common/s_llrint.c
+++ b/newlib/libm/common/s_llrint.c
@@ -93,9 +93,9 @@ long long int
if (j0 >= 52)
/* 64bit return: j0 in [52,62] */
/* 64bit return: left shift amt in [32,42] */
- result = ((long long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) |
+ result = ((long long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
/* 64bit return: right shift amt in [0,10] */
- (i1 << (j0 - 52));
+ ((long long int) i1 << (j0 - 52));
else
{
/* 64bit return: j0 in [20,51] */
diff --git a/newlib/libm/common/s_lrint.c b/newlib/libm/common/s_lrint.c
index 9d2cb7306..b37f50fd4 100644
--- a/newlib/libm/common/s_lrint.c
+++ b/newlib/libm/common/s_lrint.c
@@ -131,9 +131,9 @@ TWO52[2]={
if (j0 >= 52)
/* 64bit return: j0 in [52,62] */
/* 64bit return: left shift amt in [32,42] */
- result = ((long int) ((i0 & 0x000fffff) | 0x0010000) << (j0 - 20)) |
+ result = ((long int) ((i0 & 0x000fffff) | 0x00100000) << (j0 - 20)) |
/* 64bit return: right shift amt in [0,10] */
- (i1 << (j0 - 52));
+ ((long int) i1 << (j0 - 52));
else
{
/* 32bit return: j0 in [20,30] */
--
2.14.3

Looks right to me, but can you resend the patch as attachment, please?
Your MUA apparently replaced tabs with spaces.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Matthias J. Kannwischer

2018-05-29 13:55:00 UTC

Permalink

Hi Corinna,

sure, it's attached - sorry for that.

Best,

Matthias

Post by Corinna Vinschen
Hi Matthias,

Looks right to me, but can you resend the patch as attachment, please?
Your MUA apparently replaced tabs with spaces.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-05-29 14:02:03 UTC

Permalink

Post by Matthias J. Kannwischer
Hi Corinna,
sure, it's attached - sorry for that.
Best,
Matthias

Post by Corinna Vinschen
Hi Matthias,

Looks right to me, but can you resend the patch as attachment, please?
Your MUA apparently replaced tabs with spaces.
Thanks,
Corinna

Pushed.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat