[PATCH] mini-gmp: pass correct old_size to custom reallocate function
minux
minux.ma at gmail.com
Sat Mar 7 20:27:00 UTC 2020
All comments addressed, except as noted below.
I also fixed similar issues in mini-mpq.c changes.
On Sat, Mar 7, 2020 at 2:26 PM Niels Möller <nisse at lysator.liu.se> wrote:
> minux <minux.ma at gmail.com> writes:
> > 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.
>
> The differences are documented in mini-gmp/README (and your patch fixes
> all but one of the documented differences).
Aha, thanks for pointing that out. Removed those in the revised patch.
> > 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.
>
> Changes to mini-gmp.c look reasonable to me. Some detailed comments
> below.
>
> > In fact, I think it makes the code of mpq_get_str slightly easier to
> > read.
>
> I'm not that familiar with the mpq functions. I hope Marco can comment.
>
> > + const size_t sl = sizeof (mp_limb_t);
> > assert (size > 0);
> > - return (mp_ptr) (*gmp_reallocate_func) (old, 0, size * sizeof (mp_limb_t));
> > + return (mp_ptr) gmp_xrealloc (old, old_size * sl, size * sl);
> > +}
>
> I'd prefer repeating sizeof (mp_limb_t), instead of the local sl
> constant.
Done.
> > @@ -956,11 +964,12 @@
> > mp_limb_t d, di;
> > mp_limb_t r;
> > mp_ptr tp = NULL;
> > + mp_size_t tps = 0;
> >
> > if (inv->shift > 0)
> > {
> > /* Shift, reusing qp area if possible. In-place shift if qp == np. */
> > - tp = qp ? qp : gmp_xalloc_limbs (nn);
> > + tp = qp ? qp : gmp_xalloc_limbs (tps = nn);
>
> Please write the assignment tps = nn as a separate statement. Also, I
> think name "tn" instead of "tps" would be more consistent with
> surrounding code.
Done.
> > r = mpn_lshift (tp, np, nn, inv->shift);
> > np = tp;
> > }
> > @@ -978,7 +987,7 @@
> > qp[nn] = q;
> > }
> > if ((inv->shift > 0) && (tp != qp))
> > - gmp_free (tp);
> > + gmp_xfree_limbs (tp, tps);
>
> Can the condition be simplified to just
>
> if (tps > 0)
> gmp_xfree_limbs (tp, tps);
>
> ?
Yes, simplified.
> > @@ -4143,7 +4152,7 @@
> > size_t
> > mpz_sizeinbase (const mpz_t u, int base)
> > {
> > - mp_size_t un;
> > + mp_size_t un, oun;
> > mp_srcptr up;
> > mp_ptr tp;
> > mp_bitcnt_t bits;
> > @@ -4176,7 +4185,7 @@
> > 10. */
> > }
> >
> > - tp = gmp_xalloc_limbs (un);
> > + tp = gmp_xalloc_limbs (oun = un);
>
> I'd prefer
>
> tn = un;
>
> as a separate statement. And maybe clearer to leave un unchanged below, and
> use tn as the loop variable?
Done.
> > mpn_copyi (tp, up, un);
> > mpn_div_qr_1_invert (&bi, base);
> >
> > @@ -4189,7 +4198,7 @@
> > }
> > while (un > 0);
> >
> > - gmp_free (tp);
> > + gmp_xfree_limbs (tp, oun);
> > return ndigits;
> > }
> >
> > @@ -4199,7 +4208,7 @@
> > unsigned bits;
> > const char *digits;
> > mp_size_t un;
> > - size_t i, sn;
> > + size_t i, sn, osn = 0;
> >
> > digits = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> > if (base > 1)
> > @@ -4220,15 +4229,15 @@
> >
> > sn = 1 + mpz_sizeinbase (u, base);
> > if (!sp)
> > - sp = (char *) gmp_xalloc (1 + sn);
> > + sp = (char *) gmp_xalloc (osn = 1 + sn);
>
> I'd prefer
>
> if (!sp)
> {
> osn = 1 + sn;
> sp = (char *) gmp_xalloc (osn);
> }
> else
> osn = 0;
>
> (and drop the zero initialization at the declaration above).
Done.
> > +ret:
> > sp[sn] = '\0';
> > + if (osn) sp = gmp_xrealloc(sp, osn, sn + 1);
> > return sp;
> > }
>
> Would it make sense to also check if realloc is needed?
> if (osn && osn != sn + 1)
> gmp_xrealloc(...);
Yes, check added.
> > @@ -4305,7 +4316,7 @@
> > r->_mp_size = 0;
> > return -1;
> > }
> > - dp = (unsigned char *) gmp_xalloc (strlen (sp));
> > + dp = (unsigned char *) gmp_xalloc (dpn = strlen (sp));
>
> dpn = strlen (sp);
>
> as a separate statement. And I'm not that fond of the name dpn. Maybe
>
> sn = strlen (sp);
Done.
> and reorganize the loop as
>
> for (i = 0; i < sn; i++)
> {
> ...
> dp[i] = digit;
> }
I think the current loop looks good as is.
> > @@ -4382,7 +4393,7 @@
> > str = mpz_get_str (NULL, base, x);
> > len = strlen (str);
> > len = fwrite (str, 1, len, stream);
> > - gmp_free (str);
> > + gmp_xfree (str, len + 1);
> > return len;
> > }
>
> This seems wrong in the case of i/o errors, where fwrite may return a
> shorter item count.
Fixed.
> > --- a/mini-gmp/tests/testutils.c Thu Mar 05 00:43:26 2020 +0100
> > +++ b/mini-gmp/tests/testutils.c Sat Mar 07 13:04:26 2020 -0500
> > @@ -84,31 +84,51 @@
> > static void *
> > tu_realloc (void *p, size_t old_size, size_t new_size)
> > {
> > - size_t *block = block_check (p);
> > - block = (size_t *) realloc (block, sizeof(size_t) + new_size + sizeof(block_end));
> > + size_t *old_block = block_check (p), *block;
> > + void *new;
> > + if (old_block[0] != old_size)
> > + {
> > + fprintf (stderr, "%s:%d: bad old_size: want %ld, got %ld.\n", __FILE__, __LINE__,
> > + (long)old_block[0], (long)old_size);
> > + abort ();
> > + }
> > +
> > + /* We do not use realloc here to test custom allocators where realloc
> > + requires old_size to be set to the length of data that needs to be
> > + copied from old to the newly allocated space. */
>
> I think the above check of old_size should be enough for testing
> mini-gmp. One can still use realloc for the actual allocation, no manual
> memcpy.
Revised to use realloc instead of malloc+memcpy+free.
Thanks for the detailed review and please take another look.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mini-gmp.patch
Type: application/octet-stream
Size: 12079 bytes
Desc: not available
URL: <https://gmplib.org/list-archives/gmp-devel/attachments/20200307/04fa1230/attachment.obj>
More information about the gmp-devel
mailing list