Chez Scheme: raise catchable exception for expt with huge result

[samth]

Raise a catchable exception for expt with an integer base and bignum exponent, rather than consuming all memory and aborting. Also add a practical size limit in S_bignum to catch cases where the exponent is a large fixnum (e.g., on 64-bit).

This was found in the random testing for Typed Racket, see racket/typed-racket#1494.

[mflatt]

I see that the latest commit moves the practical-limit check to the Scheme side — thanks! — but I'm skeptical of that number. Lets assume that the magic number 40 is given a name in "cmacros.ss": maximum-plausible-bignum-bits? If I'm calculating right, 40 corresponds to a bignum that is 1/8 of a TB, and I don't see a clear reason to draw the line there. I agree that it's unlikely that anyone wants a number that large, but I think it might fit in hardware that you can buy now. Raising an error when the number is too big to fix into the representation of bignums is clearly a useful check, and that would correspond to maximum-bignum-length. Maybe I'm missing some reason that 40 is effectively a hard limit already? Otherwise, hardwiring a lower "practical" limit seems troublesome in the long run.

In the context of things like Typed Racket tests, maybe the right thing is to use custodian-memory-limit in Racket so you can pick a limit suitable for a given test suite? The expr operation exported by Racket includes a check against that limit — which may be right or may need to be refined further, but a configurable check in Racket seems like more the right place.

[samth]

A few thoughts:

1. Sorry for not seeing this response before adding more code; I will revise based on that and use the maximum-bignum-length.
2. The original issue came about with an exception that was not caught by the memory limit, see the error here: https://drdr.racket-lang.org/72141/cs/racket/share/pkgs/typed-racket-test/external/tr-random-testing.rkt A specific example that triggers the abort is (expt 2 (expt -19 11)) which this commit fixes.

More generally I'm not sure what the philosophy should be here. My thought was that safe Scheme operations should not trigger aborts, even when they try to allocate large amounts of memory. But maybe that's not the goal and we should be a lot more permissive at the cost of aborts so that if there is enough memory the operation will succeed.

[mflatt]

The original issue came about with an exception that was not caught by the memory limit,

Definitely a Racket bug, with not enough abses applied at the run-time check and compilation guards. I'll push a Racket repair for that.

[samth]

This now uses maximum-bignum-length (and I did the same for a few more operations). It's still easy to crash by using values that are too large but not maximum-bignum-length but this shouldn't break any working code.

[mflatt]

There are a bunch of 5s in the latest code that I think are the log-base-2 of (constant bigit-bits). Is that right? If so it would be better to add (define-constant log2-bigit-bits (log2 (constant bigit-bits))) to "cmacro.ss" and then use (constant log2-bigit-bits) would be much better than using 5. But I think this is going to be a moot point...

Unless I'm confused, it turns out that a fixnum can never be as large as (* (constant bigit-bits) (constant maximum-bignum-length)). This is not exactly a coincidence, because it's connected to the way tag bits work and the fact (log2 (constant bigit-bits)) is more than a couple of bits, and it seems unlikely to ever stop being true. I would be inclined to leave out the out-of-memory checks for fixnum shifts everywhere (which is all the places with 5), and maybe add an assertion somewhere (maybe in the idiot_checks function in "scheme.c") to make sure there's an alert if it somehow becomes wrong in the future.

For expt, it looks like the implementation covers the case that the first argument is an exact rational, since that recurs to expt, but I think it doesn't cover the case of an exact complex number, as in (expt 2+3i (expt 2 100)). Then again, I'm not sure how much this matters, since the calculation will take a long time to get to the point of running out of memory.

[mflatt]

This looks good to me! I've pushed a commit for small suggestions, which you can further adjust or take back out, as you see fit. (If you approve, then it will be squashed into your on merge.) The suggested commit adjusts the check to avoid potential overflow in the future, adds a not about why the check is there, and adjusts the release note slightly. I'm not sure whether the release note belongs in functionality changes or bug fixes, but I'm fine with listing it as a bug fix.

[samth]

Your adjustments look good to me.