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

[minux]

On Sat, Mar 7, 2020 at 3:42 AM Niels Möller <nisse at lysator.liu.se> wrote:
> > According to GMP documentation chapter 13 for reallocate_function:
> > "The block may be moved if necessary or if desired, and in that case
> > the smaller of old_size and new_size bytes must be copied to the new
> > location."
>
> As I understand the docs, the intention is *not* that old_size for
> realloc is the amount of data currently in use, which I agree would be
> useful. As I understand it, it is required that the old_size argument is
> same value passed as allocation size when the old block was previously
> allocated, regardless of how much is actually in use.
>
> To be concrete, if you look at GMP's _mpz_realloc function, in
> mpz/realloc.c, or passes the mpz's _mp_alloc as old_size, not the
> _mp_size field.

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

> The requirement that realloc and free gets the original allocation size
> as argument is easy in most cases, e.g., in _mpz_realloc, but causes
> additional complexity in a few cases, In particular, for mpz_get_str
> with a NULL input buffer, mini-gmp's implementation of mpz_get_str
> sometimes allocates a byte extra, and the application has no way of
> knowing that and passing the original allocation size when later
> deallocating the buffer. GMP's implementation deals with this case using
> an extra realloc, to guarantee that the allocation size equals strlen +
> 1.
>
> The reason mini-gmp behaves differently here is that (i) the old_size
> argument appear to be rarely used (because memory alocators typically
> maintains their own metadata about alloctions), and (ii) to eliminate
> some complexity from mpz_get_str.

Memory allocators generally keep track of the allocated memory's size,
except for bump-the-pointer garbage collector (GC) based memory
allocators, where they don't necessarily need to keep track of the
total size (they only care about pointers), as is the case in (older)
ghc's runtime (link above): its realloc implementation just allocates
a new block, and copy min(old_size, new_size) byte worth of data from
the old block into the newly allocated block and return. If old_size
is always passed as 0, then such clients won't work with mini-gmp.

We couldn't just argue those clients are broken, because they
implemented realloc exactly according to the docs:
"The block may be moved if necessary or if desired, and in that case
the smaller of old_size and new_size bytes must be copied to the new
location."

I agree that it's difficult to keep track of size of old in mini-gmp
(especially in mpz_get_str), but I think this patch makes mini-gmp a
viable replacement of gmp in such GC based applications. 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.

Thanks.