[PATCH] mini-gmp: pass correct old_size to custom reallocate function

[minux]

On Sat, Mar 7, 2020 at 10:20 AM Niels Möller <nisse at lysator.liu.se> wrote:
> minux <minux.ma at gmail.com> writes:
>
> > Yes, the old_size is supposed to be the size of the originally
> > allocated memory block. But the problem is that some old gmp client
> > used to rely on the old_size behave as documented:
> > https://github.com/ghc/ghc/blob/d874b8c93b737bf26c949ef7bf19fc43e335bd1f/rts/sm/Storage.c#L935
>
> Thanks, that's the first example I've seen of an application using
> old_size.
>
> In the alloc function (starting at line 913), it ends with
>
>   SET_ARR_HDR(arr, &stg_ARR_WORDS_info, CCCS, data_size_in_words);
>
>   /* and return a ptr to the goods inside the array */
>   return arr->payload;
>
> Maybe it would be easy for the realloc function to get the "arr" object
> back and get data size from here?

Yes, it's possible to modify ghc to avoid relying on old_size being
correct. My point is more about making mini-gmp a drop-in replacement
for gmp in cases where mini-gmp provides sufficient API coverage
though.

> > I consider the risk of passing a slightly smaller old_size in the case
> > of mpz_get_str is less risky because most allocations don't use
> > old_size for memory management purposes as you have observed. Maybe we should
> > document this difference for mini-gmp though.
>
> I don't really like such a subtle difference in documented behavior.
> Always passing zero is a less subtle uncompatibility, and will break
> applications depending on the GMP behavior in a more obvious way.
>
> If we change mini-gmp to be compatible with GMP here, I'd prefer to go
> all the way and make it fully compatible. But we should do that only if
> it's useful enough to warrant the extra complexity. ghc is one example,
> but it's still unclear to me (i) if it's hard or easy to fix the ghc
> allocator to use its own metadata, and (ii) what's the usecase and
> context for building ghc with mini-gmp.

I'm trying to bootstrap ghc on modern systems where I don't care
bignum performance that much, but I do care about minimizing
dependency on the build system. Using mini-gmp to replace gmp makes it
much easier to build. And mini-gmp is only missing one tiny piece to
be usable in this way.
Because we don't have separate docs for mini-gmp, and mini-gmp does
provide mp_set_memory_functions, I think it's best if it can match
gmp's behavior here to avoid confusions.

OK, patch updated. Now mini-gmp keeps track of old_size correctly and
implements the behavior exactly as documented. And I have modified
tests to ensure old_size is correct on realloc and free.
I hope the added complexity is not too much. In fact, I think it makes
the code of mpq_get_str slightly easier to read.
The patch also makes mpz_get_str and mpq_get_str conform to their gmp
docs on the size of returned string, if newly allocated ("The block
will be strlen(str)+1 bytes, that being exactly enough for the string
and null-terminator.") This too, is (implicitly) checked by the
modified test suite.
While I'm at it, also fixed a compiler warning in tests/t-mpq_str.c
about set by unused variable.

Please take another look.

Thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mini-gmp.patch
Type: application/octet-stream
Size: 10363 bytes
Desc: not available
URL: <https://gmplib.org/list-archives/gmp-devel/attachments/20200307/54d9056f/attachment-0001.obj>