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.

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

The bignum module changes behavior of Math::BigInt::bdiv() #9937

Open
p5pRT opened this issue Oct 29, 2009 · 15 comments
Open

The bignum module changes behavior of Math::BigInt::bdiv() #9937

p5pRT opened this issue Oct 29, 2009 · 15 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 29, 2009

Migrated from rt.perl.org#70109 (status was 'open')

Searchable as RT70109$

@p5pRT
Copy link
Author

p5pRT commented Oct 29, 2009

From jl_post@hotmail.com

Created by jl_post@hotmail.com

The documentation in "perldoc Math​::BigInt" says that
if $x is a Math​::BigInt object, the code​:

  $x->bdiv($y);

will change the value of $x. That is exactly what I
see when I run the following code​:

#!/usr/bin/perl

use strict;
use warnings;

use Math​::BigInt;
my $n = Math​::BigInt->new(6);

print "\$n before \$n->bdiv(2)​: $n\n"; # prints 6
$n->bdiv(2);
print "\$n after \$n->bdiv(2)​: $n\n"; # should print 3

# use bignum;

__END__

The output I get (on both Linux and Strawberry Perl
for Windows) is​:

$n before $n->bdiv(2)​: 6
$n after $n->bdiv(2)​: 3

Nothing wrong here. (This is exactly what I expect to see.)

However, if I uncomment the "use bignum;" line (that last
line of the script, right before the __END__ tag) and re-run
the code, I get this output (on both Linux and Strawberry Perl
for Windows)​:

$n before $n->bdiv(2)​: 6
$n after $n->bdiv(2)​: 6

Notice that now the second line says that $n is 6.
(It should say 3.)

Evidently just the inclusion of the "bignum" module is causing
the Math​::BigInt​::bdiv() method to behave differently (in that
it is not modifying the calling object).

I haven't found any other methods that are affected
(such as Math​::BigInt​::bmul()), any other modules that
are affected (such as Math​::BigFloat), or any other
offending modules that cause this problem (such as
"bigint").

So the bug is that simply declaring "use bignum;"
causes the Math​::BigInt​::bdiv() method to not modify
the calling object.

Perl Info

Flags:
    category=core
    severity=low

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/jromano
    LANG=en_US
    LANGUAGE=en_US:en_GB:en
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin
    PERL_BADLANG (unset)
    SHELL=/bin/tcsh

@p5pRT
Copy link
Author

p5pRT commented Nov 11, 2009

From @pjf

On Thu Oct 29 10​:21​:18 2009, jl_post@​hotmail.com wrote​:

Evidently just the inclusion of the "bignum" module is causing
the Math​::BigInt​::bdiv() method to behave differently (in that
it is not modifying the calling object).

Ouch! Nice catch, that looks like a nasty bug. I can confirm it's
still a problem under 5.10.0 and 5.10.1.

I'm marking this as something that needs attention in 5.12.0. Giving
wrong results is bad.

Thanks again for the bug report, it's very much appreciated!

  Paul

--
Paul Fenwick <pjf@​perltraining.com.au> | http​://perltraining.com.au/
Director of Training | Ph​: +61 3 9354 6001
Perl Training Australia | Fax​: +61 3 9354 2681

@p5pRT
Copy link
Author

p5pRT commented Nov 11, 2009

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

@p5pRT
Copy link
Author

p5pRT commented Nov 11, 2009

From @TJC

I confirmed this bug still exists in 5.11.1

@p5pRT
Copy link
Author

p5pRT commented Jan 20, 2010

From @iabyn

On Thu, Oct 29, 2009 at 10​:21​:18AM -0700, jl_post@​hotmail.com (via RT) wrote​:

#!/usr/bin/perl

use strict;
use warnings;

use Math​::BigInt;
my $n = Math​::BigInt->new(6);

print "\$n before \$n->bdiv(2)​: $n\n"; # prints 6
$n->bdiv(2);
print "\$n after \$n->bdiv(2)​: $n\n"; # should print 3

# use bignum;

[snip]

However, if I uncomment the "use bignum;" line (that last
line of the script, right before the __END__ tag) and re-run
the code, I get this output (on both Linux and Strawberry Perl
for Windows)​:

$n before $n->bdiv(2)​: 6
$n after $n->bdiv(2)​: 6

Notice that now the second line says that $n is 6.
(It should say 3.)

Evidently just the inclusion of the "bignum" module is causing
the Math​::BigInt​::bdiv() method to behave differently (in that
it is not modifying the calling object).

It's actually a big in BigInt.pm or BigFloat.pm; the 'use bignum' just
sets an internal flag, as can be seen below​:

  use Math​::BigInt;
  use Math​::BigFloat;

  my $fn = Math​::BigInt->new(6);
  $Math​::BigInt​::upgrade ='Math​::BigFloat'; # side-effect of 'use bignum';
  $fn->bdiv(2);

  print "result (should be 3)​: $fn\n";

Jesse, given that Big* are CPAN modules and that this bug is present in
5.8.*, I can't see that this bug should be a 5.12 showstopper.

Also, does anyone know if there's a way to transfer this to the
appropriate CPAN RT queue, since its not a core bug? Or does one just ask
the originator to re-report it?

--
That he said that that that that is is is debatable, is debatable.

@p5pRT
Copy link
Author

p5pRT commented Jan 20, 2010

From @obra

On Wed Jan 20 14​:13​:43 2010, davem wrote​:

Jesse, given that Big* are CPAN modules and that this bug is present
in 5.8.*, I can't see that this bug should be a 5.12 showstopper.

It's not a regression, but I was certainly happy to take PJF's assertion
that it's nasty enough to want a fix at face value.

Bugs in dual-lifed modules can certainly be showstoppers if they're nasty enough. Doing my own
pass through this list an hour ago, I opted not to unlink as a showstopper yet, mostly in the
hope that the dwindling size of the list would sucker some poor soul into providing us/tels with
a fix. I don't feel particularly strongly about that, though. If you want to unlink it now, I
won't object.

Also, does anyone know if there's a way to transfer this to the
appropriate CPAN RT queue, since its not a core bug? Or does one just
ask the originator to re-report it?

There is not. We've been talking to perl.org about merging the two, but we're not there yet.

@p5pRT
Copy link
Author

p5pRT commented Jan 21, 2010

From @nwc10

On Wed, Jan 20, 2010 at 10​:13​:10PM +0000, Dave Mitchell wrote​:

Also, does anyone know if there's a way to transfer this to the
appropriate CPAN RT queue, since its not a core bug? Or does one just ask
the originator to re-report it?

I've been rather hoping that sd could be taught how to do this
(specifically one way trips).

http​://syncwith.us/sd/

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jan 21, 2010

From @xdg

On Wed, Jan 20, 2010 at 5​:19 PM, Jesse via RT <perlbug-followup@​perl.org> wrote​:

Also, does anyone know if there's a way to transfer this to the
appropriate CPAN RT queue, since its not a core bug? Or does one just
ask the originator to re-report it?

There is not. We've been talking to perl.org about merging the two, but we're not there yet.

One could forward the original email to the right RT queue at
rt.cpan.org. And then "reply" to the new ticket with a link to the
rt.perl.org ticket for further discussion.

David

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2010

From @obra

Based on previous discussion and input from former pumpkings, I'm removing
this as a 5.12.0 blocker.

@jkeenan
Copy link
Contributor

jkeenan commented Oct 29, 2023

Back in October 2009 it was reported,

"Evidently just the inclusion of the "bignum" module is causing
the Math​::BigInt​::bdiv() method to behave differently (in that
it is not modifying the calling object)."

There was discussion as to the real cause of the problem.

I reported this problem upstream today at https://rt.cpan.org/Ticket/Display.html?id=150252. @pjacklam, can you take a look? Thanks.

@pjacklam
Copy link
Contributor

pjacklam commented Oct 30, 2023

The underlying cause here is upgrading and downgrading. Unlike the bigint, bigrat, and bigfloat pragmas, the bignum pragma enables upgrading and downgrading by default. The observed behaviour is how it always has been¹, and from what I understand, this is how the original author intended it to work.

Here is what happens: Division is an operation that might return a non-integer. So when upgrading is enabled in Math::BigInt, the division is handed over to the upgrade class. By default, Math::BigInt upgrades to Math::BigFloat. So it is Math::BigFloat that performs the division, using Math::BigFloat objects. When Math::BigFloat has performed a division, and downgrading is enabled, Math::BigFloat checks the result to see if it is an integer, and if it is, the result is passed to the downgrade class. By default, Math::BigFloat downgrades to Math::BigInt. So the final result is a Math::BigInt object. However, it will be a different object than the original invocand object.

The same thing happens with every Math::BigInt method that might return a non-integer, not just bdiv(). You get the same with bsqrt():

$x = Math::BigInt -> new(9); $x -> bsqrt();    # $x is still 9

I think the best solution is to improve the documentation and make it clear that this will happen, and why. The only way to make sure that the $x is never modified when upgrading and/or downgrading is enabled, is to re-bless $x from one class to another. This can be done, but from what I know of OOP, this is very bad practice. It would be possible to check whether the result can be represented exactly as an integer, and upgrade only when the result is not an integer. The downside of this, is that certain operations must be computed twice. But I am open for suggestions, corrections, arguments, … :)

¹ There were a few releases of bignum where I disabled upgrading and downgrading. This functionality was only partly implemented, according to the orginal author, and I had given up trying to understand how and when it was supposed to work. Eventually I got a grip of it and enabled it again.

@jkeenan
Copy link
Contributor

jkeenan commented Oct 30, 2023

The underlying cause here is upgrading and downgrading. Unlike the bigint, bigrat, and bigfloat pragmas, the bignum pragma enables upgrading and downgrading by default. The observed behaviour is how it always has been¹, and from what I understand, this is how the original author intended it to work.

Here is what happens: Division is an operation that might return a non-integer. So when upgrading is enabled in Math::BigInt, the division is handed over to the upgrade class. By default, Math::BigInt upgrades to Math::BigFloat. So it is Math::BigFloat that performs the division, using Math::BigFloat objects. When Math::BigFloat has performed a division, and downgrading is enabled, Math::BigFloat checks the result to see if it is an integer, and if it is, the result is passed to the downgrade class. By default, Math::BigFloat downgrades to Math::BigInt. So the final result is a Math::BigInt object. However, it will be a different object than the original invocand object.

The same thing happens with every Math::BigInt method that might return a non-integer, not just bdiv(). You get the same with bsqrt():

$x = Math::BigInt -> new(9); $x -> bsqrt();    # $x is still 9

@pjacklam, thank you for that explanation. I appreciate the time and effort you had to put into researching it.

I think the best solution is to improve the documentation and make it clear that this will happen, and why.

Yes, I think that some documentation about the existence of weird corner cases would be good.

@sisyphus
Copy link
Contributor

sisyphus commented Oct 31, 2023

The only way to make sure that the $x is modified when upgrading and/or downgrading is enabled, is to re-bless $x from one class to another. This can be done, but from what I know of OOP, this is very bad practice, ...

But this is exactly what already happens with:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x /= 2; print $x; print ref($x);"
Math::BigInt
3.5
Math::BigFloat

I personally think that's the correct behaviour, and I would expect that the exact same thing would happen with:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x->bdiv(2); print $x; print ref($x);"
Math::BigInt
7
Math::BigInt

But if we want to use bdiv() we instead need to do:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x = $x->bdiv(2); print $x; print ref($x);"
Math::BigInt
3.5
Math::BigFloat

To make it worse, these method calls will modify-in-place the object's value if there's no upgrading involved, but won't modify-in-place the value if upgrading is involved,
UPDATE - 2 examples that demonstrate this:

D:\>perl -Mbignum -wle "$x = 7; $x->badd(2.5); print $x;"
7

D:\>perl -Mbignum -wle "$x = 7; $x->badd(2); print $x;"
9

This is all goes on silently, does not DWIM, and requires that the programmer be alert to these issues.

I know that ships have sailed and bridges have been burnt.
I don't use bignum (and I haven't perused the docs,), but if I did use bignum I would hope that I use the overloaded operators rather than these troublesome method calls.

@pjacklam, if it's impractical to make any behavioural changes, then I agree that the bignum docs should prominently set out the perils of using the method calls.

Cheers,
Rob

@pjacklam
Copy link
Contributor

The only way to make sure that the $x is modified when upgrading and/or downgrading is enabled, is to re-bless $x from one class to another. This can be done, but from what I know of OOP, this is very bad practice, ...

But this is exactly what already happens with:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x /= 2; print $x; print ref($x);"
Math::BigInt
3.5
Math::BigFloat

You got me there. I didn’t expect that. It must be perl itself that takes care of this, because /= is implemented as

    '/=' => sub { scalar $_[0] -> bdiv($_[1]); },

When I look at the addresses, I see that they change:

$ perl -MScalar::Util=refaddr -Mbignum -wle \
    '$x = Math::BigInt->new(7); print refaddr($x); $x /= 2; print $x; print refaddr($x);'
42949688776
3.5
42949904040

I personally think that's the correct behaviour, and I would expect that the exact same thing would happen with:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x->bdiv(2); print $x; print ref($x);"
Math::BigInt
7
Math::BigInt

I agree. I think the fact that $x /= 2 and $x -> bdiv(2) give different results is counter-intuitive.

But if we want to use bdiv() we instead need to do:

D:\>perl -Mbignum -wle "$x = Math::BigInt->new(7); print ref($x); $x = $x->bdiv(2); print $x; print ref($x);"
Math::BigInt
3.5
Math::BigFloat

Right. The code is full of cases like $x = $x -> bdiv($y) because – in general – I don’t know whether $x is modified in-place or not. I agree that this is not ideal.

To make it worse, these method calls will modify-in-place the object's value if there's no upgrading involved, but won't modify-in-place the value if upgrading is involved, UPDATE - 2 examples that demonstrate this:

D:\>perl -Mbignum -wle "$x = 7; $x->badd(2.5); print $x;"
7

D:\>perl -Mbignum -wle "$x = 7; $x->badd(2); print $x;"
9

This is all goes on silently, does not DWIM, and requires that the programmer be alert to these issues.

Yes, I am aware of this. I believe the idea is that when you use bignum, it is to get transparent support for arbitrarily large numbers. In such cases, you don’t use method calls.

@pjacklam, if it's impractical to make any behavioural changes, then I agree that the bignum docs should prominently set out the perils of using the method calls.

I have been thinking about this, and I believe it is not as difficult to implement as I originally though. The question is how many other modules will break if I make this change. :-)

@sisyphus
Copy link
Contributor

sisyphus commented Nov 1, 2023

I believe the idea is that when you use bignum, it is to get transparent support for arbitrarily large numbers. In such cases, you don’t use method calls.

Yes, I expect that's the usual practice. Otherwise there would surely have been more complaints about this issue than just this one report from 14 years ago.

I have been thinking about this, and I believe it is not as difficult to implement as I originally thought. The question is how many other modules will break if I make this change. :-)

I actually think not many, if any, breakages will occur. (Easy for me to say, I know ;-)
It should be a very rare case that someone does $y = $x->bdiv(2) and relies on $x not being modified.
If they are relying on $x not being modified, then they need to be certain that the bdiv(2) call initiates an upgrade or downgrade of the result.

But code like $x = $x->bdiv(2) or $x = $x->bsqrt() would not be broken. (It's just that those 2 pieces of code are catching the return value, when there would no longer be any need to do that.)

(BTW, I've been allowing cross-class overloading between my Math::GMPz, Math::GMPq and Math::MPFR modules for a while now - for reasons of "fun" rather than "usefulness". Lately, I've enhanced these operations to allow "upgrading", but I don't do any "downgrading".
It's all done in the XS code - so perhaps of little relevance to the task facing you. And I don't really know whether my implementation of some of the "upgrading" overloaded operations leak memory ....)

Cheers,
Rob

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants