BBC: Blead breaks SISYPHUS/Math-MPFR-4.21.tar.gz, SISYPHUS/Math-GMPf-0.47.tar.gz, SISYPHUS/Math-Float128-0.15.tar.gz

[eserte]

t/NOK_and_POL.t in Math-MPFR-4.21 started to fail with perl 5.35.10:

2 != 1

2 != 1

3 != 1

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

4 != 2

5 != 3

5 != 3
t/NOK_and_POK.t .................. 
Failed 20/27 subtests 

Math-GMPf-0.47 and Math-Float128-0.15 have a same-named test script, which also started to fail.

[eserte]

@sisyphus FYI

[jkeenan]

Bisecting with the following invocation:

perl Porting/bisect.pl --start=v5.35.9 --end=v5.35.10 --module=Math::MPFR

... pointed to the following commit:

b1e3ba27cdcf1b4e2558f05ed4cc6780d2f208e1 is the first bad commit
commit b1e3ba27cdcf1b4e2558f05ed4cc6780d2f208e1
Author: Graham Knop <haarg@haarg.org>
Date:   Tue Mar 15 15:19:03 2022 +0100
Commit:     Paul Evans <leonerd@leonerd.org.uk>
CommitDate: Tue Mar 15 15:44:55 2022 +0000


    always prevent setting POK flag when NV values are used as strings

@haarg, @leonerd, can you take a look?

[hvds]

The test is specifically checking the NOK and POK flags on Inf and NaN values, so it is unsurprising that this change results in test failures.

Because most of the tests are checking the value of an accumulating count, it also fails on all of those tests after the first true failure: I suspect there are only 2 real test failures here.

My guess is that only the tests will need to change, but @sisyphus will need to compare against whatever he's actually trying to achieve here; I'll create an issue for the distro.

[hvds]

Created sisyphus/math-mpfr#4

[haarg]

These tests are checking what the internal flags are after using values in various forms. This is sensitive adjustments to how core handles the flags, which have been intentionally changed in 5.35.10. I'd say that these dists need to have their tests adjusted.

The tests are written in a rather strange way, so it's not trivial to propose a fix. I'll let @sisyphus look at fixing them first.

[sisyphus]

Sorry I'm a bit late to this - I've been off-grid for a few days.
I'm aware of the failure, and I'll fix it before 5.36 is released. It's not a big deal for me to deal with.
(I hadn't anticipated the recent change to the flags when an inf/nan NV is stringified.)

[sisyphus]

The problem is not quite as I initially thought it would be.
Here's a demo of the issue, in a nutshell:

use warnings;
use strict;

use Inline C =><<'EOC';

void foo(SV * in) {
  if(SvNOK(in)) printf("NOK\n");
  if(SvPOK(in)) printf("POK\n");
}

EOC

my $inf = 999 ** (999 ** 999);
my $discard = "$inf";

foo_it($inf);

sub foo_it {
  foo($_[0]); 
  print "\n";
  my $arg = shift;
  foo($arg); 
}

On perls prior to the 5.35.10 dev release, this script would output:

NOK
POK

NOK
POK

But with the release of perl-5.35.10, it outputs:

NOK

NOK
POK

Do we really want perl's shift() function to alter the flags like that ?
I expect not .... at least I hope not. (IMO, it just makes a mockery of removing the POK flag in the first place.)

However, in my tests I can, of course deal with this bizarre behaviour if I need to.

[hvds]

Do we really want perl's shift() function to alter the flags like that ?

I would have assumed not. It's not shift per se, but assignment; you can see it here:

% perl -MDevel::Peek -E '$i=999**(999**999); ()="$i"; $j=$i; Dump($_) for ($i,$j)' 2>&1 | grep FLAGS
  FLAGS = (NOK,pNOK,pPOK)
  FLAGS = (NOK,POK,pNOK,pPOK)
% 

This happens in sv_setsv_flags around sv.c:4435 - this block is entered if ssv->flags & SVp_POK:

        /* Whichever path we take through the next code, we want this true,
           and doing it now facilitates the COW check.  */
        (void)SvPOK_only(dsv);

@nwc10 is this intended behaviour?

[haarg]

This definitely looks like a bug to me.

I believe the code in the SVp_IOK section that disables the SVp_POK flag needs to be copied into the SVp_NOK section.

[nwc10]

This happens in sv_setsv_flags around sv.c:4435 - this block is entered if ssv->flags & SVp_POK:

        /* Whichever path we take through the next code, we want this true,
           and doing it now facilitates the COW check.  */
        (void)SvPOK_only(dsv);

@nwc10 is this intended behaviour?

Well, it was in 2003 (commit 120fac9) but a lot has changed since then, so I'm not so sure now.

But I think that @haarg's suggestion is right. Something like this makes all the modules pass (and the Devel::Peek testcase also show the same flags)

diff --git a/sv.c b/sv.c
index 8ddf159ada..58b21ed650 100644
--- a/sv.c
+++ b/sv.c
@@ -4547,6 +4547,16 @@ Perl_sv_setsv_flags(pTHX_ SV *dsv, SV* ssv, const I32 flags)
         }
         if (sflags & SVp_NOK) {
             SvNV_set(dsv, SvNVX(ssv));
+            if ((sflags & SVf_NOK) && !(sflags & SVf_POK)) {
+                /* Source was SVf_NOK|SVp_NOK|SVp_POK but not SVf_POK, meaning
+                   a value set as floating point and later stringified, where
+                  the value happens to be one of the few that we know aren't
+                  affected by the numeric local, hence we can cache the
+                  stringification. Currently that's  +Inf, -Inf and NaN, but
+                  conceivably we might extend this to -9 .. +9 (excluding -0).
+                  So mark destination the same: */
+                SvFLAGS(dsv) &= ~SVf_POK;
+            }
         }
         if (sflags & SVp_IOK) {
             SvIV_set(dsv, SvIVX(ssv));

I'm too tired now to write sensible tests (or a decent commit message), and I'm not sure if there are other places that have been missed.

More generally, it might seem inefficient to set the flag bit SVf_POK and then later clear it again, but

1. in the short term I didn't want to audit all that intervening flag code (with macros) to see if there's a change in behaviour by not initially setting SVf_POK
2. longer term, it feels more defensive against future bugs to clear the flag at the end, instead of relying on the intervening code to never manage to set it. (But this might be viewed as defeatist, instead of defensive)

[sisyphus]

In the modules of mine that are affected by this, I'll add some code that detects the presence of this behaviour and modifies the failing test script accordingly.
Hopefully, this will be done in such a way that does not require a rewrite if this current behaviour is altered. (But that's my responsibility, anyway.)

AFAICS it's a fairly inconsequential bug.
Even though both the POK and NOK flags are set, both the PV and NV slots contain identical values (inf/nan) so it doesn't matter which slot is used.
(In saying that, I'm ignoring any situations where infnan strings are not numified correctly or where infnan values are not stringified correctly. These situations seem to be very rare, but I won't be so bold as to declare that they don't exist.)

[sisyphus]

Fixed by #19580 (which has not yet been merged at time of writing).

[rjbs]

This (#19580) has now been merged.