op/pack.t failures with PPC long double (double double) builds

p5pRT commented 9 years ago

Migrated from rt.perl.org#125669 (status was 'resolved')

Searchable as RT125669$

p5pRT commented 9 years ago

From @sisyphus

Hi\,

The op/pack.t test script fails with both perl-5.23.1 and perl-5.22.0. (Details given here relate to perl-5.23.1\, but the failures with 5.22.0 are identical.)

See the attached pack.fail for the failing tests and their diagnostics.

Perl -V is:

#################################################### Summary of my perl5 (revision 5 version 23 subversion 1) configuration:

Platform: osname=linux\, osvers=3.2.0-4-powerpc64\, archname=ppc64-linux-thread-multi-ld uname='linux debian-sis 3.2.0-4-powerpc64 #1 smp debian 3.2.68-1+deb7u2 ppc64 gnulinux ' config_args='-des -Duse64bitint -Dusethreads -Duselongdouble -Duse64bitall -Dcc=gcc -m64 -Dprefix=/home/sisyphus-sis/bleadperl -Dusedevel' hint=recommended\, useposix=true\, d_sigaction=define useithreads=define\, usemultiplicity=define use64bitint=define\, use64bitall=define\, uselongdouble=define usemymalloc=n\, bincompat5005=undef Compiler: cc='gcc -m64'\, ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'\, optimize='-O1'\, cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.6.3'\, gccosandvers='' intsize=4\, longsize=8\, ptrsize=8\, doublesize=8\, byteorder=87654321\, doublekind=4 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16\, longdblkind=6 ivtype='long'\, ivsize=8\, nvtype='long double'\, nvsize=16\, Off_t='off_t'\, lseeksize=8 alignbytes=16\, prototype=define Linker and Libraries: ld='gcc -m64'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/powerpc-linux-gnu/4.6/include-fixed /usr/lib /lib/powerpc-linux-gnu /lib/../lib /usr/lib/powerpc-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64 libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.13.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.13' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E' cccdlflags='-fPIC'\, lddlflags='-shared -O1 -L/usr/local/lib -fstack-protector'

Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES MULTIPLICITY PERLIO_LAYERS PERL_COPY_ON_WRITE PERL_DONT_CREATE_GVSV PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_ALL USE_64_BIT_INT USE_ITHREADS USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_LONG_DOUBLE USE_PERLIO USE_PERL_ATOF USE_REENTRANT_API Built under linux Compiled at Jul 23 2015 21:10:35 @INC: ../lib /home/sisyphus-sis/bleadperl/lib/site_perl/5.23.1/ppc64-linux-thread-multi-ld /home/sisyphus-sis/bleadperl/lib/site_perl/5.23.1 /home/sisyphus-sis/bleadperl/lib/5.23.1/ppc64-linux-thread-multi-ld /home/sisyphus-sis/bleadperl/lib/5.23.1 /home/sisyphus-sis/bleadperl/lib/site_perl . ####################################################

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

pack.fail

p5pRT commented 9 years ago

From @jkeenan

On Thu Jul 23 06:47:02 2015\, sisyphus wrote:

Hi\,

The op/pack.t test script fails with both perl-5.23.1 and perl-5.22.0. (Details given here relate to perl-5.23.1\, but the failures with 5.22.0 are identical.)

See the attached pack.fail for the failing tests and their diagnostics.

Perl -V is:

#################################################### Summary of my perl5 (revision 5 version 23 subversion 1) configuration:

Platform: osname=linux\, osvers=3.2.0-4-powerpc64\, archname=ppc64-linux-thread-multi-ld uname='linux debian-sis 3.2.0-4-powerpc64 #1 smp debian 3.2.68- 1+deb7u2 ppc64 gnulinux ' config_args='-des -Duse64bitint -Dusethreads -Duselongdouble -Duse64bitall -Dcc=gcc -m64 -Dprefix=/home/sisyphus-sis/bleadperl -Dusedevel'

1. In the section of INSTALL discussing "64 bit support"\, I read:

##### Natively 64-bit systems need neither -Duse64bitint nor -Duse64bitall. On these systems\, it might be the default compilation mode .... #####

So\, to eliminate one possible source of noise\, could you re-compile without explicitly saying either '-Duse64bitint' or '-Duse64bitall'?

When you do so\, do you get the same results?

Along the same lines\, when you build an unthreaded perl\, do you get the same failures?

2. Do you know where your architecture stands with respect to the concerns raised in the "Long doubles" section of INSTALL?

##### In some systems you may be able to use long doubles to enhance the range and precision of your double precision floating point numbers (that is\, Perl's numbers). Use Configure -Duselongdouble to enable this support (if it is available).

Note that the exact format and range of long doubles varies: the most common is the x86 80-bit (64 bits of mantissa) format\, but there are others\, with different mantissa and exponent ranges. In fact\, the type may not be called "long double" at C level\, and therefore the C\ means "using floating point larger than double". #####

-- James E Keenan (jkeenan@cpan.org)

p5pRT commented 9 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 9 years ago

From @jhi

On Sunday-201508-02 8:19\, James E Keenan via RT wrote:

In fact\, the type may not be called "long double" at C level\, and therefore the C\ means "using floating point larger than double

Darn. That's wrong and that's my fault (257c99f5). That was for a while a true statement\, while I was working on the quadmath\, but it is no more true. (Though\, one could argue that for allowing more leeway in future one should leave that fibbing in.) I tried undoing the false earlier (568793b6) but missed the above spot.

As for Sisyphus' quandary: the "double double" is "long double"\, the "double double" it's just one of the many possible implementations of "long double".

p5pRT commented 9 years ago

From @jhi

On Sunday-201508-02 8:38\, Jarkko Hietaniemi wrote:

more leeway in future one should leave that fibbing in.) I tried undoing the false earlier (568793b6) but missed the above spot.

Darn\, I meant b7ce25dd. And now I made the same misquote in 93803940.

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: Jarkko Hietaniemi Sent: Sunday\, August 02\, 2015 10:38 PM To: perlbug-followup@perl.org ; sisyphus1@optusnet.com.au Subject: Re: [perl #125669] op/pack.t failures with PPC long double (double double) builds

As for Sisyphus' quandary: the "double double" is "long double"\, the "double double" it's just one of the many possible implementations of "long double".

As pointed out on wikipedia ( https://en.wikipedia.org/wiki/Long_double )\, this format doesn't conform to " IEEE floating-point standard" ( https://en.wikipedia.org/wiki/IEEE_floating-point_standard ). But it is the C "long double"\, as implemented by Debian wheezy's gcc-4.6.3 for this OS - which does not mean that perl invariably assigns the exact same value as C. There are bugs in libc and perl (and perhaps even the C compiler itself) that create discrepancies in the assignment of certain values.

Jim\, I don't have any reason to believe that the removal of -Duse64bitint and/or -Duse64bitall and/or -Dusethreads will make a difference\, but I haven't yet verified that (to the extent of specifically testing it). WRT -Duesthreads\, it made no difference to any test results for 5.20.0.

The same op/pack.t failures occurred with 5.20.0 - no regressions\, as far as I've noticed !!

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: James E Keenan via RT Sent: Sunday\, August 02\, 2015 10:19 PM To: OtherRecipients of perl Ticket #125669: Cc: perl5-porters@perl.org Subject: [perl #125669] op/pack.t failures with PPC long double (double double) builds

So\, to eliminate one possible source of noise\, could you re-compile without explicitly saying either '-Duse64bitint' or '-Duse64bitall'?

When you do so\, do you get the same results?

Yes.

Along the same lines\, when you build an unthreaded perl\, do you get the same failures?

Yes.

Cheers\, Rob

p5pRT commented 9 years ago

From @jhi

Looking at the pattern of failures\, it is something like...

for $x (qw(c s s> s\< i i\< i> l l> l\< s! s!> s!\< i! i!> i!\< l! l!> l!\< n! v! N! V! q Q j j> j\<)) { "For list ($max_neg\, -1\, 0\, 1\, $max_pos) (total -1) packed with $x unpack '%65$x' gave 0\, expected 36893488147419103231"
}

So it's the checksum computing pack/unpack\, and for 65 bits. (The previous checksum sizes\, including 63 and 64\, have passed the tests.)

p5pRT commented 9 years ago

From @jhi

My suspicion hovers somewhere around line 1760 in pp_pack.c\, this spot:

... if (checksum) { if (strchr("fFdD"\, TYPE_NO_MODIFIERS(datumtype)) || (checksum > bits_in_uv && strchr("cCsSiIlLnNUWvVqQjJ"\, TYPE_NO_MODIFIERS(datumtype))) ) {
NV trouble\, anv;

Sisyphys\, could you add

PerlIO_printf(Perl_debug_log\, "cdouble = %"NVgf"\n"\, cdouble);

and the same for anv\, whenever they have been updated?

In Perl terms\, e.g.

./perl -wle 'print unpack("%65s"\,pack("s*"\,-32768\,-1\,0\,1\,32764))' 3.68934881474190705e+19

The 3...e+19 is the 36893488147419103231 the pack.t was expecting\, but got zero.

Of especial suspicion is the modf call in there. Does that work right for these double-doubles?

p5pRT commented 9 years ago

From [Unknown Contact. See original ticket]

My suspicion hovers somewhere around line 1760 in pp_pack.c\, this spot:

... if (checksum) { if (strchr("fFdD"\, TYPE_NO_MODIFIERS(datumtype)) || (checksum > bits_in_uv && strchr("cCsSiIlLnNUWvVqQjJ"\, TYPE_NO_MODIFIERS(datumtype))) ) {
NV trouble\, anv;

Sisyphys\, could you add

PerlIO_printf(Perl_debug_log\, "cdouble = %"NVgf"\n"\, cdouble);

and the same for anv\, whenever they have been updated?

In Perl terms\, e.g.

./perl -wle 'print unpack("%65s"\,pack("s*"\,-32768\,-1\,0\,1\,32764))' 3.68934881474190705e+19

The 3...e+19 is the 36893488147419103231 the pack.t was expecting\, but got zero.

Of especial suspicion is the modf call in there. Does that work right for these double-doubles?

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: Jarkko Hietaniemi via RT Sent: Thursday\, August 06\, 2015 12:42 PM To: OtherRecipients of perl Ticket #125669: Cc: perl5-porters@perl.org Subject: [perl #125669] op/pack.t failures with PPC long double (double double) builds

My suspicion hovers somewhere around line 1760 in pp_pack.c\, this spot:

... if (checksum) { if (strchr("fFdD"\, TYPE_NO_MODIFIERS(datumtype)) || (checksum > bits_in_uv && strchr("cCsSiIlLnNUWvVqQjJ"\, TYPE_NO_MODIFIERS(datumtype))) ) { NV trouble\, anv;

Sisyphys\, could you add

PerlIO_printf(Perl_debug_log\, "cdouble = %"NVgf"\n"\, cdouble);

and the same for anv\, whenever they have been updated?

I might not get to that until the weekend.

In Perl terms\, e.g.

./perl -wle 'print unpack("%65s"\,pack("s*"\,-32768\,-1\,0\,1\,32764))' 3.68934881474190705e+19

With both perl-5.22.0 and perl-5.23.1 I get:

./perl -wle 'print unpack("%65s"\,pack("s*"\,-32768\,-1\,0\,1\,32764))' 36893488147419070464

The 3...e+19 is the 36893488147419103231 the pack.t was expecting\, but got zero.

I note that the value I got is much closer to the value you got than to the expected "36893488147419103231".

The test 694 failure diagnostics tell me: # Failed test 694 - at op/pack.t line 628 # For list (-32768\, -1\, 0\, 1\, 32767) (total -1) packed with s unpack '%65s' gave 0\, expected 36893488147419103231 # numbers test for s>

How do I then replicate that failure once perl has been installed ? I've tried:

$ perl -wle 'print unpack("%65s"\,pack("s*"\,-32768\, -1\, 0\, 1\, 32767))' 36893488147419070464

$ perl -wle 'print unpack("%65s"\,pack("s>"\,-32768\, -1\, 0\, 1\, 32767))' 36893488147419070464

$ perl -wle 'print unpack("%65s"\,pack("s"\,-32768\, -1\, 0\, 1\, 32767))' 36893488147419070464

Just what is the actual command that's returning the '0' ? Is one really expected to derive the answer to that question by digging through pack.t ?

Of especial suspicion is the modf call in there. Does that work right for these double-doubles?

Perl_modf() seems ok to me. The attached Perl_modf.pl (which actually calls Perl_modf) outputs:

1.41421356237309504880168872421 1 0.4142135623730950488016887242097

4.123105625617660549821409855974 4 0.1231056256176605498214098559741

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

Perl_modf.pl

p5pRT commented 9 years ago

From @jhi

The 3...e+19 is the 36893488147419103231 the pack.t was expecting\, but got zero.

Umm. Now this is strange (on a x86_64 system):

perl -wle 'print unpack("%65s*"\,pack("s*"\,-32768\,-1\,0\,1\,32767))' 0

(as opposed to '%64': perl -wle 'print unpack("%64s*"\,pack("s*"\,-32768\,-1\,0\,1\,32767))' 18446744073709551615)

So it seems the 0 is the really expected value with %65? If this is the case\, then pack.t is wrong in $calc_sum computation (line 600 and around). Maybe the line 711 is wrong in double-double?

711 if ($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) {

Sisyphus\, could you debug some in pack.t?

p5pRT commented 9 years ago

From [Unknown Contact. See original ticket]

The 3...e+19 is the 36893488147419103231 the pack.t was expecting\, but got zero.

Umm. Now this is strange (on a x86_64 system):

perl -wle 'print unpack("%65s*"\,pack("s*"\,-32768\,-1\,0\,1\,32767))' 0

(as opposed to '%64': perl -wle 'print unpack("%64s*"\,pack("s*"\,-32768\,-1\,0\,1\,32767))' 18446744073709551615)

So it seems the 0 is the really expected value with %65? If this is the case\, then pack.t is wrong in $calc_sum computation (line 600 and around). Maybe the line 711 is wrong in double-double?

711 if ($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) {

Sisyphus\, could you debug some in pack.t?

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: Jarkko Hietaniemi via RT Sent: Friday\, August 07\, 2015 9:02 AM To: OtherRecipients of perl Ticket #125669: Cc: perl5-porters@perl.org Subject: [perl #125669] op/pack.t failures with PPC long double (double double) builds

711 if ($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) {

Sisyphus\, could you debug some in pack.t?

Yes\, I think that line (line 611) makes assumptions that just don't hold for doubledoubles.

I've verified that the condition ($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) is not met at any stage up to (and including) test 13348. Test 13348 is the last test that fails - and I haven't checked beyond that point to see if the condition is ever met\, but I doubt that is as $max_p1 is 36893488147419103232 ( == 2 **65). I've yet to calculate the lowest positive value for $calc_sum such that $calc_sum == $calc_sum - 1. (Might be trivial - though I wouldn't be surprised if the range of such values is *not* continuous.)

Judging by the associated comments\, the underlying assumption of line 611 seems to be that 'long double' precision will be no greater than 64 bits.

I notice that the failing tests have invariably given 0\, with an expected value of 36893488147419103231 (which is $calc_sum).

But when I run the actual failing test\, I invariably get a value that is neither 0 nor 36893488147419103231. For example\, the test 13348 diagnostics tell me that the test failed because:

# For list (-2147483648\, -1\, 0\, 1\, 2147483647) (total -1) packed with j\< unpack '%65j\<' gave 0\, expected 36893488147419103231

But when I run that code from the command line I get something entirely different: $ ../perl -I../lib -wle 'print unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647));' 36893488145271619584

65-bit binary representation of 36893488145271619584 is: 11111111111111111111111111111111110000000000000000000000000000000

(That's 34 set bits followed by 31 unset bits.) It's the same for *all* of the failing tests.

Now .... it's very easy to get all of the pack.t tests to pass. We just just alter that condition at line 611 of pack.t to:

(($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) || $calc_sum >= 2 ** 64)

I think that is (maybe) a fair enough rewrite of the condition - assuming that: a) the precision of the checksum can be no greater than UV precision; b) the precision of the UV can be no greater than 64 bits.

But\, although pack.t then passes all tests\, determining that the previously failing tests are now returning 0 as expected\, perl is\, in fact\, returning a different value (namely\, 36893488145271619584).

I deduce 2 things from this: 1) pack.t isn't deriving the "gave" value directly from the command it wants to check; 2) there's a bug in in the perl source (probably the same sort of assumption that line 611 made) that's allowing the return of 65-bit values.

I'll post this off as is - though I hope to make further progress today. Any comments/corrections/pointers thus far appreciated.

Questions: Is my assumption that the checksum must fit into a UV correct ? If so\, why isn't "%x" a fatal error for x greater than UV-precision ?

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: Jarkko Hietaniemi via RT Sent: Thursday\, August 06\, 2015 12:42 PM To: OtherRecipients of perl Ticket #125669: Cc: perl5-porters@perl.org Subject: [perl #125669] op/pack.t failures with PPC long double (double double) builds

My suspicion hovers somewhere around line 1760 in pp_pack.c\, this spot:

... if (checksum) { if (strchr("fFdD"\, TYPE_NO_MODIFIERS(datumtype)) || (checksum > bits_in_uv && strchr("cCsSiIlLnNUWvVqQjJ"\, TYPE_NO_MODIFIERS(datumtype))) ) { NV trouble\, anv;

As suggested\, I replaced that section with:

if (checksum) { if (strchr("fFdD"\, TYPE_NO_MODIFIERS(datumtype)) || (checksum > bits_in_uv && strchr("cCsSiIlLnNUWvVqQjJ"\, TYPE_NO_MODIFIERS(datumtype))) ) { NV trouble\, anv;

anv = (NV) (1 \<\< (checksum & 15)); PerlIO_printf(Perl_debug_log\, "A anv = %"NVgf"\n"\, anv); while (checksum >= 16) { checksum -= 16; anv *= 65536.0; PerlIO_printf(Perl_debug_log\, "B anv = %"NVgf"\n"\, anv); } while (cdouble \< 0.0) { PerlIO_printf(Perl_debug_log\, "A cdouble = %"NVgf"\n"\, cdouble); cdouble += anv; PerlIO_printf(Perl_debug_log\, "B cdouble = %"NVgf"\n"\, cdouble); } cdouble = Perl_modf(cdouble / anv\, &trouble) * anv; PerlIO_printf(Perl_debug_log\, "C cdouble = %"NVgf"\n"\, cdouble); sv = newSVnv(cdouble); }

I then get:

$ ./perl -I../lib -wle 'print unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647));' A anv = 2 B anv = 131072 B anv = 8.58993e+09 B anv = 5.6295e+14 B anv = 3.68935e+19 A cdouble = -2.14748e+09 B cdouble = 3.68935e+19 C cdouble = 3.68935e+19 36893488145271619584

By my calculations\, the final value for anv (3.68935e+19) is\, in fact\, 36893488147419103232 ( == 2 **65). Apparently\, the initial value of cdouble (-2.14748e+09) is\, in fact\, 2147483648 ( == 2 ** 31) since 36893488147419103232 - 2147483648 gives the final output value of 36893488145271619584 (which is also the final value of cdouble output above).

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: sisyphus1@optusnet.com.au Sent: Sunday\, August 09\, 2015 3:28 PM

I notice that the failing tests have invariably given 0\, with an expected value of 36893488147419103231 (which is $calc_sum).

But when I run the actual failing test\, I invariably get a value that is neither 0 nor 36893488147419103231. For example\, the test 13348 diagnostics tell me that the test failed because:

# For list (-2147483648\, -1\, 0\, 1\, 2147483647) (total -1) packed with j\< unpack '%65j\<' gave 0\, expected 36893488147419103231

But when I run that code from the command line I get something entirely different: $ ../perl -I../lib -wle 'print unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647));' 36893488145271619584

Ooops ... the actual "gave" value is determined a few lines higher up in pack.t:

my $sum = eval {unpack "%$_$format*"\, pack "$format*"\, @_};

So I was running the wrong command. The command I should have run is:

$ ../perl -I../lib -wle 'print unpack("%65j\<*"\,pack("j\<*"\,-2147483648\,-1\,0\,1\,2147483647));' 0

Which does\, in fact\, agree with the "gave" value !! (It's a pity the diagnostic didn't mention the "*" ... but it's also a pity that I wasn't paying better attention.)

We just need to alter the pack.t code so that the calculation of the "expected" value is 0.

I was thinking again of my earlier suggestion of changing:

if ($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) { to: if (($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) || $calc_sum >= 2 ** 64) { or\, to limit this to doubledouble builds: if($calc_sum == $calc_sum - 1 && $calc_sum == $max_p1) || ($calc_sum >= 2 ** 64 && $Config{longdblkind} == 6)) {

BUT ... one problem with that solution is that\, if pack.t were ever to test the output of unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647)) then that test would fail - because pack.t would see a "gave" value of 36893488145271619584\, but would have calculated the "expected" value to be 0.

I'll see if I can come up with a patch to pack.t such that the tests would pass for both unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647)) and unpack("%65j\<*"\,pack("j\<*"\,-2147483648\,-1\,0\,1\,2147483647))

Incidentally\, the lowest value I've found for $x such that $x == $x-1 is (2**107) - ((2**53)+1). But there are numerous values greater than that for which the condition does *not* hold. I don't think the condition holds for any values lower than that. (Not yet proven to my satisfaction.)

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

-----Original Message----- From: sisyphus1@optusnet.com.au Sent: Tuesday\, August 11\, 2015 2:22 PM To: perlbug-comment@perl.org Cc: perl5-porters@perl.org Subject: Re: [perl #125669] op/pack.t failures with PPC long double (double double) builds

I'll see if I can come up with a patch to pack.t such that the tests would pass for both unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647)) and unpack("%65j\<*"\,pack("j\<*"\,-2147483648\,-1\,0\,1\,2147483647))

Disregard all that I've written previously. It turns out that there's a bug in my gcc/libc modfl() implementation\, and that bug carries through to Perl_modf().

At values sufficiently close to 1 and -1 (both above and below)\, modfl() and Perl_modf() do the wrong thing. Instead of returning a fractional value and setting the pointed-to-value to the integer part\, they do the converse. That is\, they set the pointed-to-value to the actual (fractional) value of the given argument\, and return 0. When this happens In the op/pack.t tests\, 0 is returned and the fractional value is slightly less than +1.

The attached patch to pp_pack.c works around the problem in so far as it allows pack.t to pass without any need to amend that file.

First question is\, do we bother about this ? If so\, then the next question is\, do we use the patch as is ? or should the inclusion of this additional code be dependent upon a preprocessor directive ?

With this patch in place\, I still get (as before):

../perl -I../lib -le 'print unpack("%65j\<"\,pack("j\<"\,-2147483648\,-1\,0\,1\,2147483647));' 36893488145271619584

But if I go so far as to change line 570 of pack.t from:

my $sum = eval {unpack "%$_$format*"\, pack "$format*"\, @_}; to: my $sum = eval {unpack "%$_$format"\, pack "$format"\, @_};

I get lots of failures and a hang. (Does it make sense to run such tests ?)

Cheers\, Rob

p5pRT commented 9 years ago

From @sisyphus

pp_pack.c.patch

```diff --- pp_pack.c 2015-08-09 17:37:55 +1000 +++ pp_pack.c_new 2015-08-12 01:36:11 +1000 @@ -1767,7 +1767,17 @@ } while (cdouble < 0.0) cdouble += anv; - cdouble = Perl_modf(cdouble / anv, &trouble) * anv; + cdouble = Perl_modf(cdouble / anv, &trouble); + + /* workaround powerpc doubledouble modfl bug */ + /* close to 1 and -1 cdouble is 0, and trouble is cdouble / anv */ + if(cdouble == Perl_ceil(cdouble) && trouble != Perl_ceil(trouble)) { + if(trouble > 1) trouble -= 1; /* not needed for op/pack.t to pass */ + if(trouble < -1) trouble += 1; /* not needed for op/pack.t to pass */ + cdouble = trouble; + } /* end of workaround */ + + cdouble *= anv; sv = newSVnv(cdouble); } else { ```

p5pRT commented 9 years ago

From @jhi

(the discussion on this went off-list\, but eventually Sisyphus came up with a nice fix\, I'm pasting his final analysis here in full)

--- paste ---

There's a number of things wrong with your patch. Remarkably\, you've made the same mistakes as I did when I first tried to catch and fix the errors.

Firstly\, Perl_modf() can get it wrong for values greater than 1. The problem with Perl_modf() is not limited to values less than 1 - in fact the same problem can also arise for values around -1 (both above and below)\, too.

I know of (essentially) only one problem value - namely plus or minus 36893488147419103231 / 36893488147419103232 and reciprocals. I haven't gone looking to see whether any other values cause Perl_modf to misbehave\, but I assume that if there other such problem values then they fit the same pattern of behaviour.

Case A Perl_modf(36893488147419103231 / 36893488147419103232\, &trouble); This should return its argument and set trouble to 0\, but it does the opposite of returning 0 and setting trouble to its argument.

Case B Perl_modf(36893488147419103232 / 36893488147419103231\, &trouble); This should return its argument minus 1\, and set trouble to 1. But it returns 0 and sets trouble to its argument.

Case C (-ve version of Case A) Perl_modf(- 36893488147419103231 / 36893488147419103232\, &trouble); This should return its argument and set trouble to 0\, but it does the opposite of returning 0 and setting trouble to its argument.

Case D (-ve version of Case B) Perl_modf(-36893488147419103232 / 36893488147419103231\, &trouble); This should return its argument plus 1\, and set trouble to -1. But it returns 0 and sets trouble to its argument.

Now\, as regards\, op/pack.t\, the only case we actually hit is that of Case A. I suspect that it might be possible to compose a checksum example that hits Case B - and if that's the case then I think the patch should allow for it.

Is it possible to strike Case C or Case D in the calculation of checksums ? (ie can the checksum ever be negative ?)

Your patch looks at the value of cdouble following the running of:

cdouble = Perl_modf(cdouble / anv\, &trouble) * anv;

but we need to examine what the Perl_modf() call did\, not what was done by Perl_modf() * anv. (If cdouble does need to be fixed\, we need to fix it *before* it gets multiplied by anv.)

Also\, you're seeking to detect cdouble != Perl_ceil(cdouble)\, but that just tells us that cdouble is non-integer. We need to detect the case where cdouble is integer\, but ought to be non-integer - ie cdouble == Perl_ceil(cdouble) && trouble != Perl_ceil(trouble). In fact\, I think we don't even have to look at cdouble - it should be sufficient to detect that trouble is non-integer (and that's what the latest version of my patch now does).

I like the inclusion of the LONGDOUBLE_DOUBLEDOUBLE condition\, so I've added this to my patch. (You might want to add some more elaborate explanation of what's happening to the comments contained therein.)

The attached pp_pack.c_patch is working fine for me - though there are currently no tests in the perl test suite that determine whether either of these 2 lines are doing anything useful:

+ if(cdouble > 1.0L) cdouble -= 1.0L; + if(cdouble \< -1.0L) cdouble += 1.0L;

I've put them in there so that the right thing might be done if Cases B or D (resp) are ever encountered. (Cases A and C don't need either of those conditions.) Feel free to remove those 2 lines if you want - they're absence/presence has no bearing on any current tests\, AFAIK.

In my patch\, it occurs to me that:

cdouble *= anv; sv = newSVnv(cdouble);

might better be replaced with:

sv = newSVnv(cdouble * anv);

but that would leave cdouble hanging with a different value. (I don't think it would matter\, but I decided to shy away from doing that\, just in case.)

I re-ran 'make test' in its entirety after building with the attached patch\, and there were no new failures.

p5pRT commented 9 years ago

From @jhi

Fix now in as http://perl5.git.perl.org/perl.git/commit/09b94b1f0efd8c107548a6fefcd471e9b06c2cdf

p5pRT commented 9 years ago

@jhi - Status changed from 'open' to 'resolved'

Perl / perl5