Bug 1763127 - R-bit64: bit64.so must be linked with -lm due to use of math functions
Summary: R-bit64: bit64.so must be linked with -lm due to use of math functions
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: R-bit64
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Elliott Sales de Andrade
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: ZedoraTracker 1697085 1734895
TreeView+ depends on / blocked
 
Reported: 2019-10-18 10:10 UTC by Elliott Sales de Andrade
Modified: 2019-11-15 03:54 UTC (History)
12 users (show)

Fixed In Version: R-bit64-0.9.7-8.fc31 R-bit64-0.9.7-6.fc29 R-bit64-0.9.7-6.fc30
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-11-15 03:01:15 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Elliott Sales de Andrade 2019-10-18 10:10:18 UTC
I'm trying to re-enable tests for R-bit64 on s390x, but they are still failing. At the moment, I believe I have narrowed down the issue to llroundl returning bogus results.

The line that is failing is `identical.integer64(i64[-63]*2%/%2, i64[-63])`. In a new test build [1], I added some print outs to the individual steps of this calculation. The result of the first step `i64[-63]*2` are completely wrong (see below). The implementation of this multiplication can be found in src/integer64.c in the function times_integer64_double. Basically, it does a loop over an array and does:

        if (e1 == NA_INTEGER64 || ISNAN(e2)) \
                ret = NA_INTEGER64; \
        else { \
                longret = e1 * (long double) e2; \       
                if (isnan(longret) || longret>MAX_INTEGER64){ \
                  naflag = TRUE; \
                  ret = NA_INTEGER64; \
                }else \                                         
                  ret = llroundl(longret); \
        }                          

where `e1` is a `long long`, `e2` is a `double`, `longret` is a `long double`, and `ret` is a `long long`.

If I replace the llroundl by a simple (long long) cast, then this tests passes and the results are as expected [2] (of course the build still fails in other tests that end up using llroundl).


Version-Release number of selected component (if applicable):
2.30.9000-11.fc32


How reproducible:
Due to bug 1763111, this is difficult for me to reproduce locally. I've had to resort to trying random things on koji scratch builds.


Steps to Reproduce:
1. Run R-bit64 build on s390x. By default, the output is not very verbose. The failure is on the line `identical.integer64(i64[-63]*2%/%2, i64[-63])` in the R examples.
2. In [1], I added a printout of the various stages of this calculation, which can be seen in the results below.


Actual results:
> i64[-63]
integer64
 [1] 1                   2                   4                  
 [4] 8                   16                  32                 
 [7] 64                  128                 256                
[10] 512                 1024                2048               
[13] 4096                8192                16384              
[16] 32768               65536               131072             
[19] 262144              524288              1048576            
[22] 2097152             4194304             8388608            
[25] 16777216            33554432            67108864           
[28] 134217728           268435456           536870912          
[31] 1073741824          2147483648          4294967296         
[34] 8589934592          17179869184         34359738368        
[37] 68719476736         137438953472        274877906944       
[40] 549755813888        1099511627776       2199023255552      
[43] 4398046511104       8796093022208       17592186044416     
[46] 35184372088832      70368744177664      140737488355328    
[49] 281474976710656     562949953421312     1125899906842624   
[52] 2251799813685248    4503599627370496    9007199254740992   
[55] 18014398509481984   36028797018963968   72057594037927936  
[58] 144115188075855872  288230376151711744  576460752303423488 
[61] 1152921504606846976 2305843009213693952
> i64[-63]*2
integer64
 [1] 2  2  2  2  3  3  3  3  3  3  3  3  4  4  4  4  4  4  5  5  5  5  6  6  6 
[26] 6  7  7  7  7  8  8  8  9  9  10 10 11 11 12 12 13 13 14 14 15 15 16 16 17
[51] 18 19 20 21 22 23 24 25 26 27 28 29
> i64[-63]*2%/%2
integer64
 [1] 2  2  2  2  2  3  3  3  3  3  3  3  3  4  4  4  4  4  4  5  5  5  5  6  6 
[26] 6  6  7  7  7  7  8  8  8  9  9  10 10 11 11 12 12 13 13 14 14 15 15 16 16
[51] 17 18 19 20 21 22 23 24 25 26 27 28


Expected results:
In the above printout, the second line should result in all entries doubled, and the third line should result in the same values as the first. All tests pass.

Additional information:
[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=38343402
[2] https://koji.fedoraproject.org/koji/taskinfo?taskID=38343574

Comment 1 Carlos O'Donell 2019-10-24 15:00:20 UTC
Elliot,

What we need is exactly the arguments you pass to llroundl so we can create a simple C reproducer for this.

Can you provide the value passed to llroundl, and the expected value versus result?

Even just one value is enough.

Comment 2 Elliott Sales de Andrade 2019-10-24 20:03:40 UTC
The input is just powers of two (2**0, 2**1, ..., up to 2305843009213693952), multiplied by two. The result should of course just be powers of two (2**1, 2**2, ...). Roughly speaking, if you expand the macro, and use constants, it should be:


long long e1 = 2; // or any other power of two.
double e1 = 2.0;
long double longret = e1 * (long double) e2;
if (isnan(longret) || longret>MAX_INTEGER64){
    // this case should not be true...
    naflag = TRUE;
    ret = NA_INTEGER64;
} else {
    ret = llroundl(longret);
}

and `ret` should be 4; it _is_ 4 if you replace llroundl by a (long long) cast.

Comment 4 Florian Weimer 2019-11-05 21:10:48 UTC
I tried to turn comment 2 into something that actually compiles and came up with this:

#include <stdio.h>
#include <math.h>
#include <limits.h>

int
main (void)
{
  long long e1 = 2; // or any other power of two.
  double e2 = 2.0;
  long double longret = e1 * (long double) e2;
  long long ret;
  if (isnan(longret) || longret > LONG_MAX){
    // this case should not be true...
    ret = 0;
  } else {
    ret = llroundl (longret);
  }
  printf ("%lld\n", ret);
}

Is this what you had in mind?  I built it with -O0 (to rule out compiler optimizations) and ran it against glibc-2.30.9000-16.fc32.s390x.  It prints 4 for me.  This was on a z13.

Anyway, I can reproduce this issue, but the bug is not in llround.  It is clearly called with invalid arguments:

(gdb) c
Continuing.
i64[-63]*2

Breakpoint 1, __llround (x=2) at ../sysdeps/ieee754/dbl-64/s_llround.c:31
31      {
(gdb) finish
Run till exit from #0  __llround (x=2)
    at ../sysdeps/ieee754/dbl-64/s_llround.c:31
0x000003fff9a0cec6 in times_integer64_double (e1_=<optimized out>, 
    e2_=<optimized out>, ret_=0x2aa00d6fca0) at integer64.c:334
334     integer64.c: No such file or directory.
Value returned is $1 = 2
(gdb) c
Continuing.

Breakpoint 1, __llround (x=2.125) at ../sysdeps/ieee754/dbl-64/s_llround.c:31
31      {
(gdb) finish
Run till exit from #0  __llround (x=2.125)
    at ../sysdeps/ieee754/dbl-64/s_llround.c:31
0x000003fff9a0cec6 in times_integer64_double (e1_=<optimized out>, 
    e2_=<optimized out>, ret_=0x2aa00d6fca0) at integer64.c:334
334     integer64.c: No such file or directory.
Value returned is $2 = 2
(gdb) 

The SEXPs appear to be correct, so that's not it.

Do you know by chance how to compile R extension modules without the build harness, and how to load them into an installed R interpreter?  I want to expand the macros and single-step through the code.

Comment 5 Florian Weimer 2019-11-05 21:51:22 UTC
Eh.  I see now that __llround is being called here, not llroundl, and the argument is treated incorrectly as a double value, while it's actually long double according to the assembler code.  Curiously, the PLT stub is for llroundl:

0x000003fff9a05ff0 in llroundl@plt ()
   from /usr/lib64/R/library/bit64/libs/bit64.so

But that jumps to __llround in glibc. 

Hah, found it!

# eu-readelf -d /usr/lib64/R/library/bit64/libs/bit64.so

Dynamic segment contains 31 entries:
 Addr: 0x0000000000020ae8  Offset: 0x01fae8  Link to section: [ 4] '.dynstr'
  Type              Value
  NEEDED            Shared library: [libR.so]
  NEEDED            Shared library: [libc.so.6]
  INIT              0x00000000000058b0

bit64.so is underlinked, so the reference to llroundl is unversioned:

# eu-readelf --symbols=.dynsym /usr/lib64/R/library/bit64/libs/bit64.so | grep llroundl
   33: 0000000000000000      0 NOTYPE  GLOBAL DEFAULT    UNDEF llroundl

This is bound to the base symbol version, llroundl. At that version, long double and double were still the same type, so this ends up effectively calling llround, and llroundl.

The fix is to link bit64.so with -lm.

Comment 6 Elliott Sales de Andrade 2019-11-06 03:30:12 UTC
Ah, thank you very much; I don't think I would have figured this out quickly.

Comment 7 Fedora Update System 2019-11-06 04:07:26 UTC
FEDORA-2019-f71081d6ca has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-f71081d6ca

Comment 8 Fedora Update System 2019-11-06 04:07:58 UTC
FEDORA-2019-a83a51a62b has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-a83a51a62b

Comment 9 Fedora Update System 2019-11-06 04:10:05 UTC
FEDORA-2019-5efd44a88b has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-5efd44a88b

Comment 10 Fedora Update System 2019-11-07 01:44:22 UTC
R-bit64-0.9.7-8.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-f71081d6ca

Comment 11 Fedora Update System 2019-11-07 01:50:41 UTC
R-bit64-0.9.7-6.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-5efd44a88b

Comment 12 Fedora Update System 2019-11-07 02:27:22 UTC
R-bit64-0.9.7-6.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-a83a51a62b

Comment 13 Fedora Update System 2019-11-15 03:01:15 UTC
R-bit64-0.9.7-8.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2019-11-15 03:20:36 UTC
R-bit64-0.9.7-6.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2019-11-15 03:54:43 UTC
R-bit64-0.9.7-6.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.