Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

text->float conversion gives wrong answer #8730

Closed
p5pRT opened this issue Jan 7, 2007 · 61 comments
Closed

text->float conversion gives wrong answer #8730

p5pRT opened this issue Jan 7, 2007 · 61 comments
Labels
distro-Linux type-core type-numerical

Comments

@p5pRT
Copy link

p5pRT commented Jan 7, 2007

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

Searchable as RT41202$
@p5pRT
Copy link
Author

p5pRT commented Jan 7, 2007

From zefram@fysh.org

Created by zefram@fysh.org

$ perl -lwe 'printf "%f\n", 1180591620717411303424.0'
1180591620717411172352.000000
$

The value that I put in the source is exactly 2^70. This is
trivially representable as a native float. The value that is printed
out is 2^70-2^17, which is the *next lower* floating-point value.
(The significand has 52 fractional bits; this value has exponent 69 and
all significand bits set.) 2^70-2^17 itself converts just fine​:

$ perl -lwe 'printf "%f\n", 1180591620717411172352.0'
1180591620717411172352.000000
$

Using that I can construct 2^70, which converts back to text normally​:

$ perl -lwe 'printf "%f\n", 1180591620717411172352.0+131072.0'
1180591620717411303424.000000
$

For further evidence that it is the text->float conversion, and not
float->text, that is in error, one can use my Data​::Float module to
display the actual bit pattern​:

$ perl -MData​::Float=float_hex -lwe 'print float_hex(1180591620717411303424.0)'
+0x1.fffffffffffffp+69
$ perl -MData​::Float=float_hex -lwe 'print float_hex(1180591620717411172352.0+131072.0)'
+0x1.0000000000000p+70
$

The same conversion fault occurs with runtime string->number conversion
also.

This fault does not occur with other programs doing this conversion on
the same machine​:

zsh% echo $((1180591620717411303424.0 == 1180591620717411172352.0))
0
zsh% echo $((1180591620717411303424.0-131072.0 == 1180591620717411172352.0))
1
zsh%

Perl Info 

Flags:
    category=core
    severity=medium

Site configuration information for perl v5.8.4:

Configured by Debian Project at Wed May 10 04:14:05 UTC 2006.

Summary of my perl5 (revision 5 version 8 subversion 4) configuration:
  Platform:
    osname=linux, osvers=2.6.15.6, archname=i386-linux-thread-multi
    uname='linux ernie 2.6.15.6 #1 thu mar 16 13:11:55 est 2006 i686 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i386-linux -Dprefix=/usr -Dprivlib=/usr/share/perl/5.8 -Darchlib=/usr/lib/perl/5.8 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.8.4 -Dsitearch=/usr/local/lib/perl/5.8.4 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dlibperl=libperl.so.5.8.4 -Dd_dosuid -des'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.3.5 (Debian 1:3.3.5-13)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=true, libperl=libperl.so.5.8.4
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.4:
    /etc/perl
    /usr/local/lib/perl/5.8.4
    /usr/local/share/perl/5.8.4
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.8
    /usr/share/perl/5.8
    /usr/local/lib/site_perl
    .


Environment for perl v5.8.4:
    HOME=/home/zefram
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/zefram/pub/i686-pc-linux-gnu/bin:/home/zefram/pub/common/bin:/usr/bin:/usr/X11R6/bin:/bin:/usr/local/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh

@p5pRT
Copy link
Author

p5pRT commented Nov 28, 2010

From @cpansprout

On Sun Jan 07 10​:45​:52 2007, zefram@​fysh.org wrote​:

$ perl -lwe 'printf "%f\n", 1180591620717411303424.0'
1180591620717411172352.000000
$

The value that I put in the source is exactly 2^70.

Perl’s atof implementation (c2988b2) multiplies by ten every time it
scans a new digit. This results in loss of precision. What is the best
way to tell whether an NV differs from the PV it was derived from?
Running it through sprintf internally seems like a bit much.

@p5pRT
Copy link
Author

p5pRT commented Nov 28, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Feb 2, 2011

From @samv

On Sun Nov 28 14​:44​:28 2010, sprout wrote​:

On Sun Jan 07 10​:45​:52 2007, zefram@​fysh.org wrote​:

$ perl -lwe 'printf "%f\n", 1180591620717411303424.0'
1180591620717411172352.000000
$

The value that I put in the source is exactly 2^70.

Perl’s atof implementation (c2988b2) multiplies by ten every time it
scans a new digit. This results in loss of precision. What is the best
way to tell whether an NV differs from the PV it was derived from?
Running it through sprintf internally seems like a bit much.

I performed some investigation of this "bug". No precision is lost by
multiplying a small integer in a float by 10. You can see how close it
is to being correct from the binary representation​:

perl -le 'print unpack("H*", reverse(pack("d",
1180591620717411303424.0)))'
444fffffffffffff

This is 1.fffffffffffff * 2^69 and a valid conversion for the input
according to at least ieee754-2008 and C99.

It seems true that C99 encourages implementations to perform "correct"
rounding, that is, returning the binary value which is closest to the
input value. However this is just wishful thinking - actual
implementation requires MP math; you need to convert the entire input
value at whatever precision it is given, to a binary value of at least
55 or so bits wide. And then apply the rounding rules in binary to find
out what the "correct" value is. So there are "halfway points", the
values above are one binary value and the values below are another. If
the input decimal number matches one of these halfway points exactly,
then you have to process some pretty big numbers.

As to how glibc gets it "right". I pulled apart the glibc and indeed
some "clever" person actually tried to take the advice in C99 literally;
strtod uses GMP internally to try to get this right. Yes, GMP.

But it still gets it wrong​:

http​://www.exploringbinary.com/incorrectly-rounded-conversions-in-gcc-
and-glibc/

This is apparently because the glibc strtod has a "pragmatic" limit of
something like 10 extra decimal digits before deciding to ignore the
rest. Even that is incorrect, as the writers in the above article note,
the worst case is over 700 decimal digits of precision that must be
calculated before the final answer can be known.

So they just made a string to number conversion not only use MP math and
so be very slow, but they didn't even get the "perfect" behaviour they
were seeking. "Excessive cleverness"

Practically speaking, Perl's implementation is about as good as you
could expect from a high performance solution and as far as I can tell
confirms to ieee754 rules. If you want the over-engineered version in
glibc, then build your Perl with d_strtod=1 or use a CPAN module which
implements the semantics you are after.

I suggest setting this ticket to 'rejected'

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2015

From zefram@fysh.org

Tom Hukins has run into an interesting case of this bug. He's trying
to port Perl to the Beaglebone Black dev board, which is based on an
ARMv6 processor and turns out to have some floating-point oddities.
Tom commented "I believe the OS uses software floating point ATM but
plans to move over to hardware at some point". The more legitimate of
its oddities is that it doesn't support subnormal values. It turns out
that this causes the "min double" test in opbasic/arith.t to fail​:

  try $T++, 2.2250738585072014e-308 != 0.0, 'min double';

The value shown is not particularly close to the minimum IEEE
double-precision value (which is about 4.94e-324, 2**-1074). It's trying
to get the minimum *normal* IEEE double-precision value, 2**-1022.
The decimal value in the test is fractionally above that. Because the
Beaglebone system does use IEEE double parameters, this test is looking at
a relevant threshold​: 2**-1022 is not merely its minimum positive normal
value, but actually its minimum positive value overall. In theory the
test should pass. (The test is arguably bogus because we don't otherwise
require any particular FP range, but that's a separate issue.)

However, the duff decimal->float conversion has the effect of breaking
the decimal up into a bunch of single-digit subvalues to sum, all of
which in this case are necessarily below the range of normal FP values.
On a full IEEE system, the first few are representable as subnormals,
but eventually they're not representable at all, and the conversion
gets truncated, quite aside from the rounding error. (But actually,
after the first digit they're not even converted to subnormals, but
instead are erroneously treated as zero; ISTR this being reported as
a separate bug somewhere. So overall the test value is converted as
if it were only 2.0e-308, and comes out as a subnormal value that is
proportionally quite a lot lower than the proper value.) It's still
non-zero, so the test passes.

On the Beaglebone, because subnormals can't be handled at all, all of
the single-digit subvalues are computed as zero, and so the sum is zero.
So the test fails, even though the value the test intends to try is
representable distinct from zero.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Mar 10, 2018

From @sisyphus

Le Tue, 01 Feb 2011 16​:10​:13 -0800, mugwump a écrit :
[snip]

If you want the over-engineered version in
glibc, then build your Perl with d_strtod=1

This is 7 years later, and even when HAS_STRTOD is defined with the latest devel release(perl-5.27.9) we don't get the value that glibc assigns.
And the value we do get is often wrong by more than just one ulp.

I noticed quite some time ago that, with -Dusequadmath builds of perl, there are not any known cases of perl assigning a floating point value incorrectly.
I'm not saying that all values are assigned correctly on -Dusequadmath builds - merely that I haven't been able to find any incorrect assignments on -Dusequadmath builds.

In contrast, it's amazingly easy to hit floating point values on double and (extended precision) long double builds of perl that are assigned incorrectly.

So ... what's so special about the way that the quadmath builds assign values ?
AFAICT, the quadmath builds always assign the value that Perl_strtod() returns.
But on the 'double' and 'long double' builds, the (nearly always correct) value returned by Perl_strtod() is ignored in preference to the often incorrect value returned by perl's bodgey atof calculations.

Now, it seems to me that if Perl_strtod() is good enough for quadmath builds, then it ought to be good enough for 'double' and 'long double' builds, too.
So, on Windows and Ubuntu, I tried a simplistically patched numeric.c that achieved that - and with strikingly improved results for perl-5.27.9.

The patch (attached) ensures that, if Perl_strtod is defined, assignment of values for 'double' and 'long double' builds of perl follows the same path as that for '__float128' builds.
For me, Perl_strtod is defined by default on Ubuntu and MS Windows. If Perl_strtod is not defined, then the patch should have no effect.
(Note that perl.h unconditionally defines Perl_strtod to strtoflt128 if USE_QUADMATH is defined.)

The main script I've used for testing is nv2.pl (attached).

On an *unpatched* 5.27.9, running 'perl nv2.pl 10' has always quickly hit a mis-assigned value (mostly within the first 20 iterations) and died.
On the various *patched* builds of 5.27.9 that I've tested, it has always run the full 1 million iterations without producing any output.
Of course, on the patched builds, Perl_strtod and C's strtod/strtold/strtoflt128 are the same function, so it's totally expected that the script will not die when run.
What's important, however, is that the complete absence of warnings indicates that the assigned values are also correct.

I have managed to find some instances where strtod/strtold does return an incorrect value, but only on a targeted search of values close to a power of 2.
Clearly these are libc bugs - and they are comparatively very rare. I know of only 1 such value on Ubuntu, and 3 on Windows.
As regards quadmath builds, I haven't found any instances of strtoflt128 assigning incorrectly.

With the patched numeric.c, all tests still passed on x86 windows for both 'double' and 'long double' builds.(Can't do quadmath builds on Windows.)

Annoyingly, on the Windows 'long double' build only, the patch altered the way that the numification of the string '9875'x1000 alters the internals. (This caused one Math​::MPFR test to fail, and I need to investigate it.)

On Ubuntu, patched builds produce the following test failure for both the 'double' and the 'long double' build​:

t/porting/args_assert ..........................................
# Failed test 2 - PERL_ARGS_ASSERT_GROK_ATOUV is declared but not used at porting/args_assert.t line 62
# Failed test 3 - PERL_ARGS_ASSERT_GROK_BIN is declared but not used at porting/args_assert.t line 62
# Failed test 4 - PERL_ARGS_ASSERT_GROK_HEX is declared but not used at porting/args_assert.t line 62
# Failed test 5 - PERL_ARGS_ASSERT_GROK_INFNAN is declared but not used at porting/args_assert.t line 62
# Failed test 6 - PERL_ARGS_ASSERT_GROK_NUMBER is declared but not used at porting/args_assert.t line 62
# Failed test 7 - PERL_ARGS_ASSERT_GROK_NUMBER_FLAGS is declared but not used at porting/args_assert.t line 62
# Failed test 8 - PERL_ARGS_ASSERT_GROK_NUMERIC_RADIX is declared but not used at porting/args_assert.t line 62
# Failed test 9 - PERL_ARGS_ASSERT_GROK_OCT is declared but not used at porting/args_assert.t line 62
# Failed test 10 - PERL_ARGS_ASSERT_ISINFNANSV is declared but not used at porting/args_assert.t line 62
# Failed test 11 - PERL_ARGS_ASSERT_MY_ATOF is declared but not used at porting/args_assert.t line 62
# Failed test 12 - PERL_ARGS_ASSERT_MY_ATOF2 is declared but not used at porting/args_assert.t line 62
# Failed test 13 - PERL_ARGS_ASSERT_SCAN_BIN is declared but not used at porting/args_assert.t line 62
# Failed test 14 - PERL_ARGS_ASSERT_SCAN_HEX is declared but not used at porting/args_assert.t line 62
# Failed test 15 - PERL_ARGS_ASSERT_SCAN_OCT is declared but not used at porting/args_assert.t line 62
FAILED at test 2

Other than that, all seems fine.

Is any of this at all useful ?
If so, I'll continue to look for creases, with an aim to getting them out.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Mar 10, 2018

From @sisyphus

nv2.pl

@p5pRT
Copy link
Author

p5pRT commented Mar 10, 2018

From @sisyphus

Hmmm ... I don't see the patch.
Resending it.

@p5pRT
Copy link
Author

p5pRT commented Mar 10, 2018

From @sisyphus

--- numeric.c_orig 2018-03-07 19​:31​:23 +1100
+++ numeric.c 2018-03-10 14​:28​:53 +1100
@​@​ -1107,7 +1107,7 @​@​
  return TRUE;
}

-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
STATIC NV
S_mulexp10(NV value, I32 exponent)
{
@​@​ -1203,7 +1203,7 @​@​
  }
  return negative ? value / result : value * result;
}
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */

NV
Perl_my_atof(pTHX_ const char* s)
@​@​ -1214,7 +1214,7 @​@​

  PERL_ARGS_ASSERT_MY_ATOF;

-#ifdef USE_QUADMATH
+#ifdef Perl_strtod

  Perl_my_atof2(aTHX_ s, &x);

@​@​ -1357,11 +1357,11 @​@​
{
  const char* s = orig;
  NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
  const char* send = s + strlen(orig); /* one past the last */
  bool negative = 0;
#endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
  UV accumulator[2] = {0,0}; /* before/after dp */
  bool seen_digit = 0;
  I32 exp_adjust[2] = {0,0};
@​@​ -1374,7 +1374,7 @​@​
  I32 sig_digits = 0; /* noof significant digits seen so far */
#endif

-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
  PERL_ARGS_ASSERT_MY_ATOF2;

  /* leading whitespace */
@​@​ -1391,12 +1391,12 @​@​
  }
#endif

-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
  {
  char* endp;
  if ((endp = S_my_atof_infnan(aTHX_ s, negative, send, value)))
  return endp;
- result[2] = strtoflt128(s, &endp);
+ result[2] = Perl_strtod(s, &endp);
  if (s != endp) {
  *value = negative ? -result[2] : result[2];
  return endp;

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2018

From @sisyphus

Le Fri, 09 Mar 2018 22​:47​:01 -0800, sisyphus a écrit :

Annoyingly, on the Windows 'long double' build only, the patch altered
the way that the numification of the string '9875'x1000 alters the
internals. (This caused one Math​::MPFR test to fail, and I need to
investigate it.)

Looks to me like a mingw runtime bug in strtold() - which I've reported to​:
https://sourceforge.net/p/mingw-w64/bugs/711/

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 14, 2018

From @sisyphus

On Sun, 11 Mar 2018 04​:11​:50 -0700, sisyphus wrote​:

Le Fri, 09 Mar 2018 22​:47​:01 -0800, sisyphus a écrit :

Annoyingly, on the Windows 'long double' build only, the patch altered
the way that the numification of the string '9875'x1000 alters the
internals. (This caused one Math​::MPFR test to fail, and I need to
investigate it.)

Looks to me like a mingw runtime bug in strtold() - which I've reported to​:
https://sourceforge.net/p/mingw-w64/bugs/711/

The bug turns out to be easily worked around. Attached is the patch I eventually used with perl-5.28.0.
On the systems I've tested, it works flawlessly and I've yet to find a value that gets assigned incorrectly on either "double" builds or extended precision "long double" builds or "__float128" builds of perl.
It also does not cause any of the tests (in the perl-5.28.0 test suite) to fail.

The systems I've tested have been Ubuntu-16.04 (gcc-5.4.0 &, glibc-2.23), Debian Wheezy (gcc-4.6.3 & glibc-2.13) and MS Windows (various mingw-w64 ports of gcc-4.7.0 thru to 8.1.0).

I'm therefore unaware of any valid reason that this modified patch should not be incorporated into perl-5.29.
If objections exist, please raise them so that I can attend to them.

The effects of this patch can be nullified by building perl such that HAS_STRTOD or HAS_STRTOLD is not defined. On Linux this is as simple as configuring the perl build with -Ud_strtod or -Ud_strtold.

I've given a little more detail at https://www.perlmonks.org/?node_id=1217588

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 14, 2018

From @sisyphus

--- numeric.c_orig 2018-07-01 21​:44​:17 +1000
+++ numeric.c 2018-07-01 12​:49​:47 +1000
@​@​ -1119,7 +1119,7 @​@​
  return TRUE;
}

-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
STATIC NV
S_mulexp10(NV value, I32 exponent)
{
@​@​ -1215,7 +1215,7 @​@​
  }
  return negative ? value / result : value * result;
}
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */

NV
Perl_my_atof(pTHX_ const char* s)
@​@​ -1226,7 +1226,7 @​@​

  PERL_ARGS_ASSERT_MY_ATOF;

-#ifdef USE_QUADMATH
+#ifdef Perl_strtod

  Perl_my_atof2(aTHX_ s, &x);

@​@​ -1369,11 +1369,11 @​@​
{
  const char* s = orig;
  NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
  const char* send = s + strlen(orig); /* one past the last */
  bool negative = 0;
#endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
  UV accumulator[2] = {0,0}; /* before/after dp */
  bool seen_digit = 0;
  I32 exp_adjust[2] = {0,0};
@​@​ -1386,7 +1386,7 @​@​
  I32 sig_digits = 0; /* noof significant digits seen so far */
#endif

-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
  PERL_ARGS_ASSERT_MY_ATOF2;

  /* leading whitespace */
@​@​ -1403,12 +1403,27 @​@​
  }
#endif

-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
  {
  char* endp;
  if ((endp = S_my_atof_infnan(aTHX_ s, negative, send, value)))
  return endp;
- result[2] = strtoflt128(s, &endp);
+# if defined(__MINGW64_VERSION_MAJOR) && defined(USE_LONG_DOUBLE)
+
+ /***********************************************
+ We are unable to use strtold because of
+ https://sourceforge.net/p/mingw-w64/bugs/711/
+ &
+ https://sourceforge.net/p/mingw-w64/bugs/725/
+
+ but __mingw_strtold is fine.
+ ***********************************************/
+
+ result[2] = __mingw_strtold(s, &endp);
+
+# else
+ result[2] = Perl_strtod(s, &endp);
+# endif
  if (s != endp) {
  *value = negative ? -result[2] : result[2];
  return endp;

@p5pRT
Copy link
Author

p5pRT commented Jul 14, 2018

From @jkeenan

On Sat, 14 Jul 2018 12​:22​:08 GMT, sisyphus@​cpan.org wrote​:

The bug turns out to be easily worked around. Attached is the patch I
eventually used with perl-5.28.0.

numeric.c has changed since perl-5.28.0, so your patch did not apply cleanly. There were 3 hunks rejected, preventing me from creating a smoke-me branch for testing.

Would it be possible to re-draw this patch against perl 5 blead, preferably creating the patch with 'git format-patch'?

Thank you very much.

Cheers,
Rob

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

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2018

From @sisyphus

And, of course, when I incorporate my patch into the current blead version
of numeric.c I find that re/uniprops02.t and re/uniprops03.t both fail a
number of tests. All other tests pass.
All that my patch does is to make "double" and "long double" builds follow
the same path through numeric.c as that taken by the "_float128" builds -
and it therefore comes as no surprise that the "__float128" builds
(-Dusequadmath), built using the official (ie unpatched) numeric.c also
fail those same 2 test scripts.
(I'll notify p5p of this breakage shortly.)

So ... I think I'll wait for this issue with the quadmath builds to be
resolved before investigating further. I'm quietly confident that when that
problem with the quadmath builds gets fixed, then so does the problem with
my patch.

The actual patch I used with latest blead (commit 76416d1) is attached.

It would be great if we could have my patch smoked - though perhaps it
would be pertinent to wait for the current broken state of blead (wrt
quadmath builds) to be fixed.

Thanks for showing some interest, Jim. It's much appreciated !

Cheers,
Rob

On Sun, Jul 15, 2018 at 9​:21 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

On Sat, 14 Jul 2018 12​:22​:08 GMT, sisyphus@​cpan.org wrote​:

The bug turns out to be easily worked around. Attached is the patch I
eventually used with perl-5.28.0.

numeric.c has changed since perl-5.28.0, so your patch did not apply
cleanly. There were 3 hunks rejected, preventing me from creating a
smoke-me branch for testing.

Would it be possible to re-draw this patch against perl 5 blead,
preferably creating the patch with 'git format-patch'?

Thank you very much.

Cheers,
Rob

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

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=41202

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2018

From @sisyphus

0001-patched-numeric.c.patch 
From 53be06c206a17fea83599e7607ca72d8d2d2d7c3 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Sun, 15 Jul 2018 11:39:59 +1000
Subject: [PATCH] patched numeric.c

---
 numeric.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/numeric.c b/numeric.c
index 34eb8b3..9f98311 100644
--- a/numeric.c
+++ b/numeric.c
@@ -1143,7 +1143,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1239,7 +1239,7 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
 NV
 Perl_my_atof(pTHX_ const char* s)
@@ -1250,7 +1250,7 @@ Perl_my_atof(pTHX_ const char* s)
 
     PERL_ARGS_ASSERT_MY_ATOF;
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 
     my_atof2(s, &x);
 
@@ -1400,13 +1400,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1419,7 +1419,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1436,13 +1436,28 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         if ((endp = S_my_atof_infnan(aTHX_ s, negative, send, value)))
             return endp;
         endp = send;
-        result[2] = strtoflt128(s, &endp);
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(USE_LONG_DOUBLE)
+
+        /***********************************************
+         We are unable to use strtold because of
+          https://sourceforge.net/p/mingw-w64/bugs/711/
+          &
+          https://sourceforge.net/p/mingw-w64/bugs/725/
+
+         but __mingw_strtold is fine.
+         ***********************************************/
+
+        result[2] = __mingw_strtold(s, &endp);
+
+#  else
+        result[2] = Perl_strtod(s, &endp);        
+#  endif
         if (s != endp) {
             *value = negative ? -result[2] : result[2];
             return endp;
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2018

From @sisyphus

And, of course, when I incorporate my patch into the current blead version
of numeric.c I find that re/uniprops02.t and re/uniprops03.t both fail a
number of tests. All other tests pass.
All that my patch does is to make "double" and "long double" builds follow
the same path through numeric.c as that taken by the "_float128" builds -
and it therefore comes as no surprise that the "__float128" builds
(-Dusequadmath), built using the official (ie unpatched) numeric.c also
fail those same 2 test scripts.
(I'll notify p5p of this breakage shortly.)

So ... I think I'll wait for this issue with the quadmath builds to be
resolved before investigating further. I'm quietly confident that when that
problem with the quadmath builds gets fixed, then so does the problem with
my patch.

The actual patch I used with latest blead (commit 76416d1) is attached.

It would be great if we could have my patch smoked - though perhaps it
would be pertinent to wait for the current broken state of blead (wrt
quadmath builds) to be fixed.

Thanks for showing some interest, Jim. It's much appreciated !

Cheers,
Rob

On Sun, Jul 15, 2018 at 9​:21 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

On Sat, 14 Jul 2018 12​:22​:08 GMT, sisyphus@​cpan.org wrote​:

The bug turns out to be easily worked around. Attached is the patch I
eventually used with perl-5.28.0.

numeric.c has changed since perl-5.28.0, so your patch did not apply
cleanly. There were 3 hunks rejected, preventing me from creating a
smoke-me branch for testing.

Would it be possible to re-draw this patch against perl 5 blead,
preferably creating the patch with 'git format-patch'?

Thank you very much.

Cheers,
Rob

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

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=41202

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2018

From @sisyphus

0001-patched-numeric.c.patch 
From 53be06c206a17fea83599e7607ca72d8d2d2d7c3 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Sun, 15 Jul 2018 11:39:59 +1000
Subject: [PATCH] patched numeric.c

---
 numeric.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/numeric.c b/numeric.c
index 34eb8b3..9f98311 100644
--- a/numeric.c
+++ b/numeric.c
@@ -1143,7 +1143,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1239,7 +1239,7 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
 NV
 Perl_my_atof(pTHX_ const char* s)
@@ -1250,7 +1250,7 @@ Perl_my_atof(pTHX_ const char* s)
 
     PERL_ARGS_ASSERT_MY_ATOF;
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 
     my_atof2(s, &x);
 
@@ -1400,13 +1400,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1419,7 +1419,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1436,13 +1436,28 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         if ((endp = S_my_atof_infnan(aTHX_ s, negative, send, value)))
             return endp;
         endp = send;
-        result[2] = strtoflt128(s, &endp);
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(USE_LONG_DOUBLE)
+
+        /***********************************************
+         We are unable to use strtold because of
+          https://sourceforge.net/p/mingw-w64/bugs/711/
+          &
+          https://sourceforge.net/p/mingw-w64/bugs/725/
+
+         but __mingw_strtold is fine.
+         ***********************************************/
+
+        result[2] = __mingw_strtold(s, &endp);
+
+#  else
+        result[2] = Perl_strtod(s, &endp);        
+#  endif
         if (s != endp) {
             *value = negative ? -result[2] : result[2];
             return endp;
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2018

From @jkeenan

On Sun, 15 Jul 2018 04​:29​:42 GMT, sisyphus359@​gmail.com wrote​:

And, of course, when I incorporate my patch into the current blead version
of numeric.c I find that re/uniprops02.t and re/uniprops03.t both fail a
number of tests. All other tests pass.
All that my patch does is to make "double" and "long double" builds follow
the same path through numeric.c as that taken by the "_float128" builds -
and it therefore comes as no surprise that the "__float128" builds
(-Dusequadmath), built using the official (ie unpatched) numeric.c also
fail those same 2 test scripts.
(I'll notify p5p of this breakage shortly.)

On Linux, I got a lot of failures in 3 files​:

#####
re/uniprops02.t (Wstat​: 65280 Tests​: 33506 Failed​: 302)
  Failed tests​: 22498-22499, 22501, 22503, 22570, 26098
  26129-26136, 26202, 26274, 26312, 26346
  26348, 26350, 26352, 26418, 26420, 26422
  26424, 26439, 26490, 26492, 26494, 26496
  26532, 26561-26568, 26582, 26597, 26599
  26601, 26603, 26621, 26634, 26636, 26638-26640
  26706, 26708-26710, 26712, 26741, 26764
  26777-26784, 27272-27273, 27306, 27308
  27310, 27312, 27378, 27380, 27382, 27384
  27649-27656, 27686-27687, 27689, 27691
  27722, 27724, 27726, 27728, 27994, 28035
  28066, 28306, 28338, 28379-28380, 28410
  28446, 28463-28470, 28482, 28484, 28486
  28488, 28962, 29010, 29012, 29014-29016
  29028, 29081, 29154, 29226, 29228, 29230
  29232, 29268, 29297-29298, 29300, 29302
  29304, 29334, 29336, 29370, 29372, 29374
  29376, 29411, 29427, 29442, 29444, 29446
  29448, 29481, 29483, 29514, 29555-29556
  29585-29588, 29590, 29592, 29623-29624
  29641, 29658, 29660, 29662, 29664, 29697
  29730, 29765, 29771, 29783, 29802, 29804
  29806-29808, 29840, 29842, 29844, 29857
  29873-29874, 29876, 29878, 29880, 29933
  29945, 29981, 29984-29985, 30005, 30017-30018
  30020, 30022, 30024, 30042, 30090, 30092
  30094, 30096, 31810, 31882, 31935, 31954
  31989, 31993, 32025-32026, 32028, 32030
  32032, 32084, 32098, 32100-32102, 32104
  32170, 32172, 32174, 32176, 32242, 32244
  32246, 32248, 32314, 32386, 32458, 32460-32462
  32464, 32494-32495, 32497, 32499, 32530
  32532, 32534, 32536, 32565-32566, 32601
  32674, 32710, 32728, 32746-32748, 32750
  32752, 32800, 32818-32820, 32822, 32824
  33053, 33060, 33089-33090, 33092-33094
  33096, 33161-33166, 33168, 33183, 33203
  33233-33240, 33287, 33306-33308, 33310
  33312, 33342, 33346, 33378-33380, 33382
  33384, 33413-33414, 33416, 33418, 33420
  33450, 33452, 33454, 33456, 33486, 33491
  Non-zero exit status​: 255
re/uniprops03.t (Wstat​: 65280 Tests​: 33346 Failed​: 113)
  Failed tests​: 196, 198, 200, 202, 268-269, 345, 559, 1647-1648
  2006, 2080, 2149, 2152, 2224-2225, 2291
  2293-2294, 3212-3215, 3358, 3392, 3432
  3445, 3464, 3536, 3538, 3540, 3542, 3556
  3608, 3680, 3682, 3684, 3686, 3751-3752
  3754, 3756, 3758, 3774, 3791, 3824-3826
  3828, 3830, 3846, 3860, 3862-3864, 3866
  3896, 3898, 3900, 3902, 3936, 3938, 3968
  3970, 3972, 3974, 4007-4009, 4040, 4042
  4044, 4046, 4282, 4312, 4314-4316, 4318
  4348, 4384, 4386, 4388, 4390, 4421-4422
  4456, 4458-4462, 4493, 4509-4516, 4528
  4530, 4532-4534, 4548, 4564, 4568, 4600
  4602, 4604, 4606
  Non-zero exit status​: 255
../lib/locale.t (Wstat​: 0 Tests​: 682 Failed​: 5)
  Failed tests​: 427-428, 432, 435, 449
Files=2661, Tests=1165509, 264 wallclock secs (88.37 usr 10.40 sys + 536.50 cusr 29.09 csys = 664.36 CPU)
Result​: FAIL
makefile​:844​: recipe for target 'test_harness' failed
#####

branch​: smoke-me/jkeenan/sisyphus/41202-text-float

So ... I think I'll wait for this issue with the quadmath builds to be
resolved before investigating further. I'm quietly confident that when that
problem with the quadmath builds gets fixed, then so does the problem with
my patch.

The actual patch I used with latest blead (commit 76416d1) is attached.

It would be great if we could have my patch smoked - though perhaps it
would be pertinent to wait for the current broken state of blead (wrt
quadmath builds) to be fixed.

Thanks for showing some interest, Jim. It's much appreciated !

Cheers,
Rob

On Sun, Jul 15, 2018 at 9​:21 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

On Sat, 14 Jul 2018 12​:22​:08 GMT, sisyphus@​cpan.org wrote​:

The bug turns out to be easily worked around. Attached is the patch I
eventually used with perl-5.28.0.

numeric.c has changed since perl-5.28.0, so your patch did not apply
cleanly. There were 3 hunks rejected, preventing me from creating a
smoke-me branch for testing.

Would it be possible to re-draw this patch against perl 5 blead,
preferably creating the patch with 'git format-patch'?

Thank you very much.

Cheers,
Rob

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

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=41202

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

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2018

From @sisyphus

On Sun, 15 Jul 2018 06​:14​:33 -0700, jkeenan wrote​:

On Linux, I got a lot of failures in 3 files​:

#####
re/uniprops02.t
(Wstat​: 65280 Tests​: 33506 Failed​: 302)
Failed tests​: 22498-22499, 22501, 22503, 22570, 26098
26129-26136, 26202, 26274, 26312, 26346
26348, 26350, 26352, 26418, 26420, 26422
26424, 26439, 26490, 26492, 26494, 26496
26532, 26561-26568, 26582, 26597, 26599
26601, 26603, 26621, 26634, 26636, 26638-26640
26706, 26708-26710, 26712, 26741, 26764
26777-26784, 27272-27273, 27306, 27308
27310, 27312, 27378, 27380, 27382, 27384
27649-27656, 27686-27687, 27689, 27691
27722, 27724, 27726, 27728, 27994, 28035
28066, 28306, 28338, 28379-28380, 28410
28446, 28463-28470, 28482, 28484, 28486
28488, 28962, 29010, 29012, 29014-29016
29028, 29081, 29154, 29226, 29228, 29230
29232, 29268, 29297-29298, 29300, 29302
29304, 29334, 29336, 29370, 29372, 29374
29376, 29411, 29427, 29442, 29444, 29446
29448, 29481, 29483, 29514, 29555-29556
29585-29588, 29590, 29592, 29623-29624
29641, 29658, 29660, 29662, 29664, 29697
29730, 29765, 29771, 29783, 29802, 29804
29806-29808, 29840, 29842, 29844, 29857
29873-29874, 29876, 29878, 29880, 29933
29945, 29981, 29984-29985, 30005, 30017-30018
30020, 30022, 30024, 30042, 30090, 30092
30094, 30096, 31810, 31882, 31935, 31954
31989, 31993, 32025-32026, 32028, 32030
32032, 32084, 32098, 32100-32102, 32104
32170, 32172, 32174, 32176, 32242, 32244
32246, 32248, 32314, 32386, 32458, 32460-32462
32464, 32494-32495, 32497, 32499, 32530
32532, 32534, 32536, 32565-32566, 32601
32674, 32710, 32728, 32746-32748, 32750
32752, 32800, 32818-32820, 32822, 32824
33053, 33060, 33089-33090, 33092-33094
33096, 33161-33166, 33168, 33183, 33203
33233-33240, 33287, 33306-33308, 33310
33312, 33342, 33346, 33378-33380, 33382
33384, 33413-33414, 33416, 33418, 33420
33450, 33452, 33454, 33456, 33486, 33491
Non-zero exit status​: 255
re/uniprops03.t
(Wstat​: 65280 Tests​: 33346 Failed​: 113)
Failed tests​: 196, 198, 200, 202, 268-269, 345, 559, 1647-1648
2006, 2080, 2149, 2152, 2224-2225, 2291
2293-2294, 3212-3215, 3358, 3392, 3432
3445, 3464, 3536, 3538, 3540, 3542, 3556
3608, 3680, 3682, 3684, 3686, 3751-3752
3754, 3756, 3758, 3774, 3791, 3824-3826
3828, 3830, 3846, 3860, 3862-3864, 3866
3896, 3898, 3900, 3902, 3936, 3938, 3968
3970, 3972, 3974, 4007-4009, 4040, 4042
4044, 4046, 4282, 4312, 4314-4316, 4318
4348, 4384, 4386, 4388, 4390, 4421-4422
4456, 4458-4462, 4493, 4509-4516, 4528
4530, 4532-4534, 4548, 4564, 4568, 4600
4602, 4604, 4606
Non-zero exit status​: 255
../lib/locale.t
(Wstat​: 0 Tests​: 682 Failed​: 5)
Failed tests​: 427-428, 432, 435, 449
Files=2661, Tests=1165509, 264 wallclock secs (88.37 usr 10.40 sys +
536.50 cusr 29.09 csys = 664.36 CPU)
Result​: FAIL
makefile​:844​: recipe for target 'test_harness' failed
#####

branch​: smoke-me/jkeenan/sisyphus/41202-text-float

That looks similar to what I got, except I didn't notice any locale.t.
Things are much better now, at blead commit 6d37e91.

The numeric.c patch that I have (against the numeric.c at that commit) is attached as 0002-patched-numeric.c.patch.
Let me know if you want a patch against a different stage of numeric.c.

All is working fine for me again - and I hope you see the same improvement.

Note that -Dusequadmath builds of perl should be identical, irrespective of whether this patch is applied. This is most desirable because quadmath builds already assign floating point values correctly, straight out of the box.
It's only the "double" and "long double" builds that can benefit from this patch - and then only if Perl_strtod is defined.

Having applied the patch, if you then build perl configured with -Ud_strtod (on a "double" build) or -Ud_strtold (on a "long double" build) then you should effectively disable the patch. (This can be useful for comparison purposes.)

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2018

From @sisyphus

0002-patched-numeric.c.patch 
From 64d72c34c593bd17e0db25466201a42d358b2866 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Mon, 16 Jul 2018 13:59:19 +1000
Subject: [PATCH] patched numeric.c

---
 numeric.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/numeric.c b/numeric.c
index b608615..6de333e 100644
--- a/numeric.c
+++ b/numeric.c
@@ -1143,7 +1143,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1239,7 +1239,7 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
 NV
 Perl_my_atof(pTHX_ const char* s)
@@ -1250,7 +1250,7 @@ Perl_my_atof(pTHX_ const char* s)
 
     PERL_ARGS_ASSERT_MY_ATOF;
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 
     my_atof2(s, &x);
 
@@ -1400,13 +1400,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1419,7 +1419,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1436,7 +1436,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         char* copy = NULL;
@@ -1454,7 +1454,22 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
             s = copy + (s - orig);
         }
 
-        result[2] = strtoflt128(s, &endp);
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(USE_LONG_DOUBLE)
+
+        /***********************************************
+         We are unable to use strtold because of
+          https://sourceforge.net/p/mingw-w64/bugs/711/
+          &
+          https://sourceforge.net/p/mingw-w64/bugs/725/
+
+         but __mingw_strtold is fine.
+         ***********************************************/
+
+        result[2] = __mingw_strtold(s, &endp);
+
+#  else
+        result[2] = Perl_strtod(s, &endp);        
+#  endif
 
         /* If we created a copy, 'endp' is in terms of that.  Convert back to
          * the original */
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2018

From @sisyphus

I should probably provide an up-to-date script that checks floating point assignments for correctness.
The one I'm now using is attached as atonv.pl. (Some of my earlier attempts did not handle subnormal values correctly.)

The script requires Math-MPFR-4.03, and that module needs to have been built against mpfr-3.1.6 or later - best to build against the current stable mpfr-4.0.1 if possible.
Running the script as, say, "perl atonv.pl 324 100000" will select a hundred thousand random values in the range 1e-325 to 10e324 and record any that don't assign correctly.

I'd be interested to hear of any mis-assignments that it catches on perls that define $Config{d_strtod} or $Config{d_strtold} && to which the patch under consideration has been applied.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2018

From @sisyphus

atonv.pl

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From @sisyphus

The version of numeric.c in the smoke-me/jkeenan/sisyphus/41202-text-float branch looks wrong to me. I don't think it includes the corrections that Karl made in commits d94e901 and 6d37e91

Attached is the numeric.c patch against current blead (as at commit b2247a8...)

Could this please be applied to the smoke-me/jkeenan/sisyphus/41202-text-float branch.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From @sisyphus

0003-patched-numeric.c.patch 
From 4b3b7f7acf7e121ffc87c844fa2b7c8bad4f2871 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Thu, 19 Jul 2018 21:10:35 +1000
Subject: [PATCH] patched numeric.c

---
 numeric.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/numeric.c b/numeric.c
index e776f73..40d464a 100644
--- a/numeric.c
+++ b/numeric.c
@@ -1145,7 +1145,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1241,7 +1241,7 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
 NV
 Perl_my_atof(pTHX_ const char* s)
@@ -1252,7 +1252,7 @@ Perl_my_atof(pTHX_ const char* s)
 
     PERL_ARGS_ASSERT_MY_ATOF;
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 
     my_atof2(s, &x);
 
@@ -1402,13 +1402,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1421,7 +1421,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1438,7 +1438,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         char* copy = NULL;
@@ -1456,7 +1456,22 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
             s = copy + (s - orig);
         }
 
-        result[2] = strtoflt128(s, &endp);
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(USE_LONG_DOUBLE)
+
+        /***********************************************
+         We are unable to use strtold because of
+          https://sourceforge.net/p/mingw-w64/bugs/711/
+          &
+          https://sourceforge.net/p/mingw-w64/bugs/725/
+
+         but __mingw_strtold is fine.
+         ***********************************************/
+
+        result[2] = __mingw_strtold(s, &endp);
+
+#  else
+        result[2] = Perl_strtod(s, &endp);        
+#  endif
 
         /* If we created a copy, 'endp' is in terms of that.  Convert back to
          * the original */
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From [Unknown Contact. See original ticket]

The version of numeric.c in the smoke-me/jkeenan/sisyphus/41202-text-float branch looks wrong to me. I don't think it includes the corrections that Karl made in commits d94e901 and 6d37e91

Attached is the numeric.c patch against current blead (as at commit b2247a8...)

Could this please be applied to the smoke-me/jkeenan/sisyphus/41202-text-float branch.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From @jkeenan

On Thu, 19 Jul 2018 11​:48​:24 GMT, sisyphus@​cpan.org wrote​:

The version of numeric.c in the smoke-me/jkeenan/sisyphus/41202-text-
float branch looks wrong to me. I don't think it includes the
corrections that Karl made in commits
d94e901 and
6d37e91

Attached is the numeric.c patch against current blead (as at commit
b2247a8...)

Could this please be applied to the smoke-me/jkeenan/sisyphus/41202-
text-float branch.

Cheers,
Rob

Replacement branch for smoke-testing​:

smoke-me/jkeenan/sisyphus/41202-2nd-text-float

However, when I tried that locally, I encountered previously unseen failures in lib/locale.t​:

#####
not ok 427 Verify that a different locale radix works when doing "==" with a constant
not ok 428 Verify that a different locale radix works when doing "==" with a scalar
...
not ok 432 Verify that "==" with a scalar and an intervening sprintf still works in inner no locale
...
...
not ok 435 Verify that after a no-locale block, a different locale radix still works when doing "==" with a scalar and an intervening sprintf
...
not ok 449 Verify atof with locale radix and negative exponent

Failed 5/682 subtests

Test Summary Report

../lib/locale.t (Wstat​: 0 Tests​: 682 Failed​: 5)
  Failed tests​: 427-428, 432, 435, 449
Files=1, Tests=682, 1 wallclock secs ( 0.06 usr 0.01 sys + 0.65 cusr 0.00 csys = 0.72 CPU)
Result​: FAIL
#####

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

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From @khwilliamson

On 07/19/2018 05​:48 AM, sisyphus@​cpan.org via RT wrote​:

The version of numeric.c in the smoke-me/jkeenan/sisyphus/41202-text-float branch looks wrong to me. I don't think it includes the corrections that Karl made in commits d94e901 and 6d37e91

Attached is the numeric.c patch against current blead (as at commit b2247a8...)

Could this please be applied to the smoke-me/jkeenan/sisyphus/41202-text-float branch.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2018

From @khwilliamson

On 07/19/2018 05​:48 AM, sisyphus@​cpan.org via RT wrote​:

The version of numeric.c in the smoke-me/jkeenan/sisyphus/41202-text-float branch looks wrong to me. I don't think it includes the corrections that Karl made in commits d94e901 and 6d37e91

Attached is the numeric.c patch against current blead (as at commit b2247a8...)

Could this please be applied to the smoke-me/jkeenan/sisyphus/41202-text-float branch.

Cheers,
Rob

Again, I wonder if the Mingw stuff should be moved into perl.h? So
that the #ifdef's for that are all in perl.h, and Perl_strtod behaves
consistently everywhere

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2018

From @sisyphus

On Thu, 19 Jul 2018 07​:33​:43 -0700, public@​khwilliamson.com wrote​:

Again, I wonder if the Mingw stuff should be moved into perl.h? So
that the #ifdef's for that are all in perl.h, and Perl_strtod behaves
consistently everywhere

Ok - I'll move the MinGW stuff relating to Perl_strtod into perl.h.
As it stands,Perl_strtod is called only in numerc.c, but that might not always be the case - and, besides, it does make better organizational sense if it's in perl.h.

I'll post again later with the relevant patches.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2018

From @sisyphus

On Thu, 19 Jul 2018 17​:38​:58 -0700, sisyphus@​cpan.org wrote​:

I'll post again later with the relevant patches.

AS of current blead (commit cb57a25) attached are the relevant patches to perl.h and numeric.c.
These patches will remain relevant until perl.h and/or numeric.c are altered.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0002-numeric_c.patch 
From dad23802050e0088442e08523664e0429a3298d7 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 10:58:08 +1000
Subject: [PATCH 2/4] numeric_c

---
 numeric.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/numeric.c b/numeric.c
index e776f73..2a340d7 100644
--- a/numeric.c
+++ b/numeric.c
@@ -1145,7 +1145,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1241,7 +1241,7 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
 NV
 Perl_my_atof(pTHX_ const char* s)
@@ -1252,7 +1252,7 @@ Perl_my_atof(pTHX_ const char* s)
 
     PERL_ARGS_ASSERT_MY_ATOF;
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 
     my_atof2(s, &x);
 
@@ -1402,13 +1402,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1421,7 +1421,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1438,7 +1438,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         char* copy = NULL;
@@ -1456,7 +1456,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
             s = copy + (s - orig);
         }
 
-        result[2] = strtoflt128(s, &endp);
+        result[2] = Perl_strtod(s, &endp);
 
         /* If we created a copy, 'endp' is in terms of that.  Convert back to
          * the original */
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0003-perl_h.patch 
From 9dc1423d02ea4057c568552b011ef86ff0ba47cd Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 10:58:45 +1000
Subject: [PATCH 3/4] perl_h

---
 perl.h | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/perl.h b/perl.h
index f4b146d..16bcf23 100644
--- a/perl.h
+++ b/perl.h
@@ -1565,6 +1565,28 @@ EXTERN_C char *crypt(const char *, const char *);
 #define PERL_SNPRINTF_CHECK(len, max, api) STMT_START { if ((max) > 0 && (Size_t)len > (max)) Perl_croak_nocontext("panic: %s buffer overflow", STRINGIFY(api)); } STMT_END
 
 #ifdef USE_QUADMATH
+#  define Perl_strtod(s, e) strtoflt128(s, e)
+#elif defined(HAS_LONG_DOUBLE) && defined(USE_LONG_DOUBLE)
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(HAS_STRTOLD)
+      /***********************************************
+       We are unable to use strtold because of
+        https://sourceforge.net/p/mingw-w64/bugs/711/
+        &
+        https://sourceforge.net/p/mingw-w64/bugs/725/
+
+       but __mingw_strtold is fine.
+      ***********************************************/
+#    define Perl_strtod(s, e) __mingw_strtold(s, e)
+#  elif defined(HAS_STRTOLD)
+#    define Perl_strtod(s, e) strtold(s, e)
+#  elif defined(HAS_STRTOD)
+#    define Perl_strtod(s, e) (NV)strtod(s, e) /* Unavoidable loss. */
+#  endif
+#elif defined(HAS_STRTOD)
+#  define Perl_strtod(s, e) strtod(s, e)
+#endif
+
+#ifdef USE_QUADMATH
 #  define my_snprintf Perl_my_snprintf
 #  define PERL_MY_SNPRINTF_GUARDED
 #elif defined(HAS_SNPRINTF) && defined(HAS_C99_VARIADIC_MACROS) && !(defined(DEBUGGING) && !defined(PERL_USE_GCC_BRACE_GROUPS)) && !defined(PERL_GCC_PEDANTIC)
@@ -6474,17 +6496,6 @@ expression, but with an empty argument list, like this:
 
 #define Atof				my_atof
 
-#ifdef USE_QUADMATH
-#  define Perl_strtod(s, e) strtoflt128(s, e)
-#elif defined(HAS_LONG_DOUBLE) && defined(USE_LONG_DOUBLE)
-#  if defined(HAS_STRTOLD)
-#    define Perl_strtod(s, e) strtold(s, e)
-#  elif defined(HAS_STRTOD)
-#    define Perl_strtod(s, e) (NV)strtod(s, e) /* Unavoidable loss. */
-#  endif
-#elif defined(HAS_STRTOD)
-#  define Perl_strtod(s, e) strtod(s, e)
-#endif
 
 #if !defined(Strtol) && defined(USE_64_BIT_INT) && defined(IV_IS_QUAD) && \
 	(QUADKIND == QUAD_IS_LONG_LONG || QUADKIND == QUAD_IS___INT64)
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

proto.h

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

Attached are the latest patches

Ignore the proto.h attachment.
Attached (with any luck) will be the patch to proto.h.

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0004-proto_h.patch 
From 53de570889de38a9cc7e915f195ab2bd4c2de97a Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 10:59:39 +1000
Subject: [PATCH 4/4] proto.h

---
 proto.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proto.h b/proto.h
index b94a47d..2b4c647 100644
--- a/proto.h
+++ b/proto.h
@@ -4327,7 +4327,7 @@ STATIC void	S_validate_suid(pTHX_ PerlIO *rsfp);
 	assert(rsfp)
 #  endif
 #endif
-#if !defined(USE_QUADMATH)
+#if !defined(Perl_strtod)
 #  if defined(PERL_IN_NUMERIC_C)
 STATIC NV	S_mulexp10(NV value, I32 exponent);
 #  endif
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @khwilliamson

On 07/31/2018 07​:07 PM, sisyphus@​cpan.org via RT wrote​:

On Thu, 26 Jul 2018 06​:37​:30 -0700, sisyphus@​cpan.org wrote​:

Just found another couple of spots (one in embed.h, one in proto.h)
where s/USE_QUADMATH/Perl_strtod/ ought to be done.

I've since made those changes to proto.h and embed.h. I think they do no more than silence a couple of compiler warnings. I also had to move the Perl_strtod definitions a bit close to the start of perl.h - otherwise embed.h and proto.h are included before Perl_strtod was defined.

The changes cause porting/regen.t to fail tests 37 and 38 because (respectively) proto.h and embed.h have changed - not that there's anything wrong with those changes.
How do I make my alterations to embed.h and proto.h without being subjected to such failures ?

embed.fnc should be patched with the necessary #ifdef's. Then
regenerating things will cause proto.h and embed.h to fall into line.

And I now also get (on Linux) porting/args_assert.t failures about symbols that were declared but not used.
Can I take it as gospel that those symbols were in fact "declared but not used" - or might it simply be that args_assert.t requires some alterations ?

I think it's pretty close to gospel.

I don't know whether that will account for the lib/locale.t failures,

It doesn't.
Those failures can best be demonstrated by running the following on a perl whose locale radix character is a comma​:

use warnings;
use locale;
$m = "3.14e+9" + 0;
$n = "3,14e+9" + 0;
print "\$m is $m\n";
print "\$n is $n\n";
__END__

On recent perls and current bleadperl (non-quadmath builds) it outputs​:
$m is 3140000000
$n is 3140000000

But with my patched perl it outputs
$m is 3140000000
$n is 3

The problem appears to be that, although perls Atof takes the locale radix character into account, Perl_strtod does not.
Quadmath builds of perl (which use Perl_strtod) avoid these failures by skipping the (5) tests that would otherwise fail. The same approach also works fine for my patched "double" and "long double" builds - though I wonder if such a concealment of a change of behaviour might be frowned upon ;-)

I'm disappointed to learn that these builds were added with such
concealment.

Which of the issues just mentioned need to be fixed before my proposed changes to perl.h, numeric.c, embed.h, and proto.h can be applied to perl.

Attached are the latest patches

I have pushed a branch that I think may add locale handling to quadmath.
  I'm having trouble getting quadmath to compile on my machine. Please
try it to see how it works. You probably can just add your patches on
top of it.

https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/khw-locale.

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=41202

1 similar comment
@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @khwilliamson

On 07/31/2018 07​:07 PM, sisyphus@​cpan.org via RT wrote​:

On Thu, 26 Jul 2018 06​:37​:30 -0700, sisyphus@​cpan.org wrote​:

Just found another couple of spots (one in embed.h, one in proto.h)
where s/USE_QUADMATH/Perl_strtod/ ought to be done.

I've since made those changes to proto.h and embed.h. I think they do no more than silence a couple of compiler warnings. I also had to move the Perl_strtod definitions a bit close to the start of perl.h - otherwise embed.h and proto.h are included before Perl_strtod was defined.

The changes cause porting/regen.t to fail tests 37 and 38 because (respectively) proto.h and embed.h have changed - not that there's anything wrong with those changes.
How do I make my alterations to embed.h and proto.h without being subjected to such failures ?

embed.fnc should be patched with the necessary #ifdef's. Then
regenerating things will cause proto.h and embed.h to fall into line.

And I now also get (on Linux) porting/args_assert.t failures about symbols that were declared but not used.
Can I take it as gospel that those symbols were in fact "declared but not used" - or might it simply be that args_assert.t requires some alterations ?

I think it's pretty close to gospel.

I don't know whether that will account for the lib/locale.t failures,

It doesn't.
Those failures can best be demonstrated by running the following on a perl whose locale radix character is a comma​:

use warnings;
use locale;
$m = "3.14e+9" + 0;
$n = "3,14e+9" + 0;
print "\$m is $m\n";
print "\$n is $n\n";
__END__

On recent perls and current bleadperl (non-quadmath builds) it outputs​:
$m is 3140000000
$n is 3140000000

But with my patched perl it outputs
$m is 3140000000
$n is 3

The problem appears to be that, although perls Atof takes the locale radix character into account, Perl_strtod does not.
Quadmath builds of perl (which use Perl_strtod) avoid these failures by skipping the (5) tests that would otherwise fail. The same approach also works fine for my patched "double" and "long double" builds - though I wonder if such a concealment of a change of behaviour might be frowned upon ;-)

I'm disappointed to learn that these builds were added with such
concealment.

Which of the issues just mentioned need to be fixed before my proposed changes to perl.h, numeric.c, embed.h, and proto.h can be applied to perl.

Attached are the latest patches

I have pushed a branch that I think may add locale handling to quadmath.
  I'm having trouble getting quadmath to compile on my machine. Please
try it to see how it works. You probably can just add your patches on
top of it.

https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/khw-locale.

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=41202

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

On Tue, 31 Jul 2018 20​:47​:53 -0700, public@​khwilliamson.com wrote​:

I have pushed a branch that I think may add locale handling to
quadmath.

Yes, that's looking good here as a quadmath build (built with "-Duse64bitall -Dusequadmath").
There's no sign of the locale problems that I see with blead.
Your khw-locale branch hadn't made any changes to lib/locale.t - so I replaced all 5 occurrences of "if($Config{usequadmath})" with "if(0)" and it still passed.
And "3,14e+9"+0 is now being evaluated correctly.

The only test failure on the quadmath build was lib/File/Copy.t which is a separate issue (https://rt-archive.perl.org/perl5/Ticket/Display.html?id=133377).

I then stuck my earlier perl.h and numeric.c changes on top of that branch - didn't bother with the proto.h and embed.h patches as that's only going to confuse things at the moment.
I then rebuilt with "-Duse64bitall" (nvtype is double).
All tests passed on this Ubuntu system with the (previously) troublesome locale setting !!

I then re-built the quadmath build, in case my additional changes might've upset it. (Thankfully, they had not.)

I also checked a uselongdouble 64-bit build on Windows, too, and I'll check other builds as time permits.

Everything looks good so far.

Attached are the patches I applied to your smoke-me/khw-locale branch.
(The patch for lib/locale.t disables the quadmath special casing - but I ought to re-work it before it's ready for blead.)

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

On Wed, 01 Aug 2018 07​:04​:30 -0700, sisyphus@​cpan.org wrote​:

Attached are the patches I applied to your smoke-me/khw-locale branch.

Jesus ... where the hell did they go :-(

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0001-lib-locale.t-no-longer-special-case-quadmath-builds.patch 
From bc4ee85df9568998ee2b744275c95ce52c905b87 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 22:29:57 +1000
Subject: [PATCH 1/3] lib/locale.t - no longer special case quadmath builds

---
 lib/locale.t | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/locale.t b/lib/locale.t
index 17931c8..b6c597f 100644
--- a/lib/locale.t
+++ b/lib/locale.t
@@ -2167,7 +2167,7 @@ foreach my $Locale (@Locale) {
     my $first_c_test = $locales_test_number;
 
     $test_names{++$locales_test_number} = 'Verify that a different locale radix works when doing "==" with a constant';
-    if ($Config{usequadmath}) {
+    if (0) {
         print "# Skip: no locale radix with usequadmath ($test_names{$locales_test_number})\n";
         report_result($Locale, $locales_test_number, 1);
     } else {
@@ -2176,7 +2176,7 @@ foreach my $Locale (@Locale) {
     }
 
     $test_names{++$locales_test_number} = 'Verify that a different locale radix works when doing "==" with a scalar';
-    if ($Config{usequadmath}) {
+    if (0) {
         print "# Skip: no locale radix with usequadmath ($test_names{$locales_test_number})\n";
         report_result($Locale, $locales_test_number, 1);
     } else {
@@ -2198,7 +2198,7 @@ foreach my $Locale (@Locale) {
     $test_names{$locales_test_number} = 'Verify that "==" with a scalar still works in inner no locale';
 
     $test_names{++$locales_test_number} = 'Verify that "==" with a scalar and an intervening sprintf still works in inner no locale';
-    if ($Config{usequadmath}) {
+    if (0) {
         print "# Skip: no locale radix with usequadmath ($test_names{$locales_test_number})\n";
         report_result($Locale, $locales_test_number, 1);
     } else {
@@ -2218,7 +2218,7 @@ foreach my $Locale (@Locale) {
     $problematical_tests{$locales_test_number} = 1;
 
     $test_names{++$locales_test_number} = 'Verify that after a no-locale block, a different locale radix still works when doing "==" with a scalar and an intervening sprintf';
-    if ($Config{usequadmath}) {
+    if (0) {
         print "# Skip: no locale radix with usequadmath ($test_names{$locales_test_number})\n";
         report_result($Locale, $locales_test_number, 1);
     } else {
@@ -2465,7 +2465,7 @@ foreach my $Locale (@Locale) {
             }
         }
 
-        if ($Config{usequadmath}) {
+        if (0) {
             print "# Skip: no locale radix with usequadmath ($Locale)\n";
             report_result($Locale, $locales_test_number, 1);
         } else {
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0002-perl.h-mingw-w64-builds-use-__mingw_strtold-instead-.patch 
From 4957c7e870ada9f7eb48b73527a43ef41d8387a1 Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 22:32:00 +1000
Subject: [PATCH 2/3] perl.h - mingw-w64 builds use __mingw_strtold instead of
 strtold

---
 perl.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 perl.h

diff --git a/perl.h b/perl.h
old mode 100644
new mode 100755
index f4b146d..1fda07c
--- a/perl.h
+++ b/perl.h
@@ -6477,7 +6477,17 @@ expression, but with an empty argument list, like this:
 #ifdef USE_QUADMATH
 #  define Perl_strtod(s, e) strtoflt128(s, e)
 #elif defined(HAS_LONG_DOUBLE) && defined(USE_LONG_DOUBLE)
-#  if defined(HAS_STRTOLD)
+#  if defined(__MINGW64_VERSION_MAJOR) && defined(HAS_STRTOLD)
+      /***********************************************
+       We are unable to use strtold because of
+        https://sourceforge.net/p/mingw-w64/bugs/711/
+        &
+        https://sourceforge.net/p/mingw-w64/bugs/725/
+
+       but __mingw_strtold is fine.
+      ***********************************************/
+#    define Perl_strtod(s, e) __mingw_strtold(s, e)
+#  elif defined(HAS_STRTOLD)
 #    define Perl_strtod(s, e) strtold(s, e)
 #  elif defined(HAS_STRTOD)
 #    define Perl_strtod(s, e) (NV)strtod(s, e) /* Unavoidable loss. */
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @sisyphus

0003-numeric.c-assign-floating-point-values-with-Perl_str.patch 
From 73684184c4de1a2af71b76e0dae624f72830bf2d Mon Sep 17 00:00:00 2001
From: sisyphus <sisyphus1@optusnet.com.au>
Date: Wed, 1 Aug 2018 22:33:38 +1000
Subject: [PATCH 3/3] numeric.c - assign floating point values with Perl_strtod
 instead of perl's Atof

---
 numeric.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 numeric.c

diff --git a/numeric.c b/numeric.c
old mode 100644
new mode 100755
index 128dac5..dffeed0
--- a/numeric.c
+++ b/numeric.c
@@ -1145,7 +1145,7 @@ Perl_grok_atoUV(const char *pv, UV *valptr, const char** endptr)
     return TRUE;
 }
 
-#ifndef USE_QUADMATH
+#ifndef Perl_strtod
 STATIC NV
 S_mulexp10(NV value, I32 exponent)
 {
@@ -1241,9 +1241,9 @@ S_mulexp10(NV value, I32 exponent)
     }
     return negative ? value / result : value * result;
 }
-#endif /* #ifndef USE_QUADMATH */
+#endif /* #ifndef Perl_strtod */
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
 #  define ATOF(s, x) my_atof2(s, &x)
 #  else
 #  define ATOF(s, x) Perl_atof2(s, x)
@@ -1406,13 +1406,13 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
 {
     const char* s = orig;
     NV result[3] = {0.0, 0.0, 0.0};
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     const char* send = s + ((len != 0)
                            ? len
                            : strlen(orig)); /* one past the last */
     bool negative = 0;
 #endif
-#if defined(USE_PERL_ATOF) && !defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) && !defined(Perl_strtod)
     UV accumulator[2] = {0,0};	/* before/after dp */
     bool seen_digit = 0;
     I32 exp_adjust[2] = {0,0};
@@ -1425,7 +1425,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     I32 sig_digits = 0; /* noof significant digits seen so far */
 #endif
 
-#if defined(USE_PERL_ATOF) || defined(USE_QUADMATH)
+#if defined(USE_PERL_ATOF) || defined(Perl_strtod)
     PERL_ARGS_ASSERT_MY_ATOF3;
 
     /* leading whitespace */
@@ -1442,7 +1442,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
     }
 #endif
 
-#ifdef USE_QUADMATH
+#ifdef Perl_strtod
     {
         char* endp;
         char* copy = NULL;
@@ -1460,7 +1460,7 @@ Perl_my_atof3(pTHX_ const char* orig, NV* value, STRLEN len)
             s = copy + (s - orig);
         }
 
-        result[2] = strtoflt128(s, &endp);
+        result[2] = Perl_strtod(s, &endp);
 
         /* If we created a copy, 'endp' is in terms of that.  Convert back to
          * the original */
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2018

From @khwilliamson

On 08/01/2018 08​:15 AM, sisyphus@​cpan.org via RT wrote​:

On Wed, 01 Aug 2018 07​:04​:30 -0700, sisyphus@​cpan.org wrote​:

Attached are the patches I applied to your smoke-me/khw-locale branch.

Jesus ... where the hell did they go :-(

I pushed your patches onto my branch for smoking.

@p5pRT
Copy link
Author

p5pRT commented Aug 2, 2018

From @sisyphus

I pushed your patches onto my branch for smoking.

I lost my connection to most of the internet a few hours ago, so I can't
see the latest smoke offerings.

I did see one report that failed both t/run/locale.t and
ext/POSIX/t/posix.t.
I think there's a chance that they will be fixed by #133417.
The need for those patches became evident to me only as a result of trying
to build with my patches && the -Ud_strtod configure option - and I'm
guessing that particular smoke did not define Perl_strtod (for whatever
reason).

I also saw t/porting/exec-bit.t failure because perl.h or numeric.c had the
exec bit set.
Was there something wrong about those patches that led to this happening ?
I'm forever having to chmod the exec bit away when I fiddle with those
files, but I didn't expect the same to afflict the smoke-me/khw-locale
branch.

Can't offer much more until I can find a way to get back to viewing those
smoker results.
My browser doesn't seem to have cached them.

The smoke-me/khw-locale branch is fine for me with double, long double and
quadmath builds on Ubuntu, and double and long double builds on Windows.

Cheers,
Rob

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2018

From @khwilliamson

On 08/02/2018 02​:45 AM, sisyphus wrote​:

I pushed your patches onto my branch for smoking.

I lost my connection to most of the internet a few hours ago, so I can't
see the latest smoke offerings.

I did see one report that failed both t/run/locale.t and
ext/POSIX/t/posix.t.
I think there's a chance that they will be fixed by #133417.
The need for those patches became evident to me only as a result of
trying to build with my patches && the -Ud_strtod configure option - and
I'm guessing that particular smoke did not define Perl_strtod (for
whatever reason).

I also saw t/porting/exec-bit.t failure because perl.h or numeric.c had
the exec bit set.
Was there something wrong about those patches that led to this happening ?
I'm forever having to chmod the exec bit away when I fiddle with those
files, but I didn't expect the same to afflict the smoke-me/khw-locale
branch.

Can't offer much more until I can find a way to get back to viewing
those smoker results.
My browser doesn't seem to have cached them.

The smoke-me/khw-locale branch is fine for me with double, long double
and quadmath builds on Ubuntu, and double and long double builds on Windows.

Cheers,
Rob

The latest smoke reports are very encouraging, with two of Dave's
patches applied (one in blead).

http​://perl.develop-help.com/?b=smoke-me%2Fkhw-locale

  Rob has sent me some minor cleanup patches, so I'm ready to apply this
to blead, and close this 11 year old ticket.

I think Dave's "HiRes​: don't truncate nanosec utime" should go in at the
same time.

Dave, shall I apply that?

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2018

From @iabyn

On Wed, Aug 08, 2018 at 08​:47​:46AM -0600, Karl Williamson wrote​:

I think Dave's "HiRes​: don't truncate nanosec utime" should go in at the
same time.

Dave, shall I apply that?

Yes please.

--
I thought I was wrong once, but I was mistaken.

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2018

From @khwilliamson

This has been fixed by the patch furnished by sisyphus, commit
ce6f496
-
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2018

@khwilliamson - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this as completed May 22, 2019
This was referenced Dec 19, 2019
Closed
Closed
Closed
@hvds hvds mentioned this issue Feb 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
distro-Linux type-core type-numerical
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@p5pRT