test failures with latest bleadperl

[hvds]

cpan-testers is showing all fails for perl-5.35.8 and .9. I'm not sure, but "Wstat 24" may mean that it received:

#define SIGXCPU     24  /* CPU time limit exceeded.  */

I note that perl got an update to bigint-0.64 around that time, so I suspect the problem lies there, this is just a heads up with 5.36 due shortly.

[update: fixed url markup]

[hvds]

Also reported to the bignum queue as https://rt.cpan.org/Ticket/Display.html?id=141772

[hvds]

Confirmed that it's caused by the upgrade to bignum-0.64.

[hvds]

Ah, from the https://metacpan.org/dist/bignum/changes at v0.60:

   - The bignum pragma now converts every numeric constant to a Math::BigFloat
     object.

[sisyphus]

The failing test script works fine with bignum-0.64 if the constant 100199294509778143137521762187425301691197073534078445671945250753109628678272 , which has now become a Math::BigFloat object, is rewritten as:

Math::BigInt->new(100199294509778143137521762187425301691197073534078445671945250753109628678272)

I guess it's just that integer division is required here, not floating-point division.

However, even with bignum-0.51, I was surprised to see:

C:\>perl -Mbignum -le "$x = 7; $y = 2; print $x / $y;"
3.5

$x and $y are Math::BigInt objects, but floating-point division has apparently been done, and a Math::BigFloat object returned.
That makes me wonder how my change to 70-rt-bignum.t actually achieved the desired result.
I see these comments by the author at the top of the file:

#  1) make sure every divide and bdiv is coerced back to an integer.
#  2) turn off upgrade in input validation.

I suspect that one or both of these objectives have been defeated by the fact that the constants are now starting off as Math::BigFloat objects, not as the expected Math::BigInt objects.

UPDATE: I think that this problematic test script should use bigint; instead of use bignum;.
Loading a pragma that promotes floating point division, when your code relies on integer division seems wrong to me, in addition to being imprudent.

[hvds]

Digging a bit more, I find that it is actually hanging in _powmod, because we enter it (from _miller_rabin_2) with $power a large bigfloat. As such, the $power >>= 1 in the loop proceeds to fractional values and never hits zero:

power=471134818610491420777276313970073849275
power=235567409305245710388638156985036924637.5
power=117783704652622855194319078492518462318.8
...
power=1.384540794380789479399211556605101773272
power=0.692270397190394739699605778302550886636
...

I think that this problematic test script should use bigint; instead of use bignum;

I guess that depends on what it's actually trying to test - the testfile was introduced at 2990405, presumably because some MPU users were using bignum in their code. Before the change in bignum-0.60, I think using bignum was probably a perfectly reasonable thing to do.

[hvds]

Arguably a/the bug here is actually Math::BigFloat's handling of >>. Perl's native floats always yield integers on >>.

[pjacklam]

Arguably a/the bug here is actually Math::BigFloat's handling of >>. Perl's native floats always yield integers on >>.

Some years ago I changed Math::BigFloat's behaviour so that >>/brsft() and <</blsft() always returned an integer, but people were furious, so I changed it back.

[pjacklam]

Ah, from the https://metacpan.org/dist/bignum/changes at v0.60:

   - The bignum pragma now converts every numeric constant to a Math::BigFloat
     object.

Upgrading and downgrading is now disabled by default. The functionality is still present, but now it must be enabled explicitly.

The original author (TELS) considered the functionality experimental. The documentation in bignum version 0.54 says

    The entire upgrading/downgrading is still experimental and might
    not work as you expect or may even have bugs.

The functionality for upgrading and downgrading is buggy and inconsistent. It can cause numbers to be upgraded and downgraded in an endless loop – in addition to other problems. After spending countless hours trying to fix upgrading/downgrading stuff, I gave up.

Now, with bigint enabled, every number is a Math::BigInt, with bignum every number is a Math::BigFloat, and with bigrat every number is a Math::BigRat. There is no upgrading or downgrading by default.

[sisyphus]

Update; I posted this before I saw @pjacklam's post above. But I don't see any need for me to correct this post - except perhaps for where I've said that "BigInts can be promoted to "BigFloats".

Firstly, if you're going to overload << and >> in a floating-point arithmetic environment, then having them multiply/divide by 2 ** $shift (which is effectively what also happens in the integer arithmetic environment) seems the sanest choice to me.

Secondly, given that under the bignum pragma, BigInts can be promoted to BigFloats, I don't think anyone can complain that an integer constant is assigned to a Math::BigFloat.
And I'd probably suggest that one simply avoid the bignum pragma whenever that behaviour poses an issue that one is not content to deal with.
Sure, that handling of the constant is unexpected behaviour (IMO, at least) - but I'm suggesting that it's allowable because of the terms and conditions under which the bignum pragma operates.
(Of course, I don't know the actual ins and outs of those "terms and conditions" ... ;-)

In short, @pjacklam, I don't see any need for you to change anything further.

However, I do not always look at things correctly.
And I'm rarely, if ever, the best-informed.

@hvds ++ for the thorough digging.

[hvds]

@danaj I hope you have a chance to look at this and decide on a course of action. Currently, the test suite will hang only for people that have upgraded their bignum package to v0.60 or later; but perl-5.36 is due before long, and the test suite will hang for everyone on that perl (assuming it ships with bignum-0.64 as currently merged).

Given @pjacklam's comments above it seems unlikely that anything will change in bignum or Math::BigFloat in this regard, so I'd suggest releasing MPU-0.74 with only the minimal changes required to get around this problem - given the size of the backlog currently in github, if that all ends up getting released as 0.74 it'll leave people nothing to fall back to if it causes problems.

[pjacklam]

One option is to revert the changes in bignum, so that bignum upgrades and downgrades as before, and instead introduce a new pragma called bigflt (or bigfloat) which converts every constant into a Math::BigFloat without any upgrading or downgrading. I guess that will probably cause less trouble for others who rely on the upgrading and downgrading being present by default. Let me know what you think.

[hvds]

One option is to revert the changes in bignum, so that bignum upgrades and downgrades as before, and instead introduce a new pragma called bigflt (or bigfloat) which converts every constant into a Math::BigFloat without any upgrading or downgrading. I guess that will probably cause less trouble for others who rely on the upgrading and downgrading being present by default. Let me know what you think.

A bigfloat would make a lot of sense to me. I had thought exactly that earlier, but didn't propose it in part because I believed you were already set on the current approach, and in part because I'm not the customer: I'm unlikely ever to use either, though I do very occasionally use bigint for a one-liner.

[danaj]

Sorry for being so late to this. This test script is finding errors as it is meant to, even though it sure would be easier to not test for these things :). As hvds commented, it's intentionally using bignum which we assume creates a BigFloat that also will return true for $n->is_int. What we'd like to do is immediately turn these into bigints that are unable to upgrade to floats, and continue. This is done inside the input validation function. This test is trying to find cases where this isn't happening correctly.

This test is passing for the code on github for me. The test is calling straight into the Pollard Rho function, bypassing the usual front end. The CPAN code does validation in the factor front end but not in the individual factoring algorithm functions. The github code adds validation in the individual functions.

[hvds]

@danaj @pjacklam It's gone rather quiet on this, is there an expectation that we'll be able to release some sort of fix for this before the perl-5.36 release due in May? Is there any consensus on what a fix would look like?