Re: [PATCH 3/9] gmp-utils: New API to simply use of GMP's integer/rational/float objects

[Simon Marchi <simark@simark.ca>]

On 2020-11-08 1:30 a.m., Joel Brobecker wrote:
> This API was motivated by a number of reasons:
>   - GMP's API does not handle "long long" and "unsigned long long",
>     so using LONGEST and ULONGEST is not straightforward;
>   - Automate the need to initialize GMP objects before use, and
>     clear them when no longer used.
>
> However, this API grew also to help with similar matter such
> as formatting to a string, and also reading/writing fixed-point
> values from byte buffers.
>
> Dedicated unit testing is also added.

Here are some comments.  Most of them are really just suggestions, what you
have looks good to me.  The suggestions can easily be implemented later, so you
don't have to re-do your patch series.

> +extern void _initialize_gmp_utils ();

You can drop "extern".

> +
> +void
> +_initialize_gmp_utils ()
> +{
> +  /* Tell GMP to use GDB's memory management routines.  */
> +  mp_set_memory_functions (xmalloc, xrealloc_for_gmp, xfree_for_gmp);

What you have is fine, but if you prefer you could also use lambda
functions, since they are trivial wrappers:

  /* Tell GMP to use GDB's memory management routines.  */
  mp_set_memory_functions (xmalloc,
			   [] (void *ptr, size_t old_size, size_t new_size)
			   {
			     return xrealloc (ptr, new_size);
			   },
			   [] (void *ptr, size_t size)
			   {
			     return xfree (ptr);
			   });

> +}
> diff --git a/gdb/gmp-utils.h b/gdb/gmp-utils.h
> new file mode 100644
> index 0000000..8a4fbfe
> --- /dev/null
> +++ b/gdb/gmp-utils.h
> @@ -0,0 +1,282 @@
> +/* Miscellaneous routines making it easier to use GMP within GDB's framework.
> +
> +   Copyright (C) 2019-2020 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GMP_UTILS_H
> +#define GMP_UTILS_H
> +
> +#include "defs.h"
> +
> +/* Include <stdio.h> and <stdarg.h> ahead of <gmp.h>, so as to get
> +   access to GMP's various formatting functions.  */
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <gmp.h>
> +#include "gdbsupport/traits.h"
> +
> +/* Same as gmp_asprintf, but returning a convenient wrapper type.  */
> +
> +gdb::unique_xmalloc_ptr<char> gmp_string_asprintf (const char *fmt, ...);

I don't know how gmp_string_asprintf will be used, but would it make
sense to make it return an std::string?  Does gmp_sprintf support
passing NULL as BUF, so that you could replicate what is done in
string_printf?

> +
> +/* A class to make it easier to use GMP's mpz_t values within GDB.  */
> +
> +struct gdb_mpz
> +{
> +  mpz_t val;

I don't know what's coming in the following patches, but would it work
to make the field private and only use it through methods?  Having it
totally encapsulated would make me more confident that the callers don't
do anything they are not supposed to with it.

There would need to be methods for arithmetic operations that we use
(e.g. mpz_add), but we don't have to add methods for all existing
functions in gmp, just the ones we use.  And I presume we don't use that
many.

> +
> +  /* Constructors.  */
> +  gdb_mpz () { mpz_init (val); }
> +
> +  explicit gdb_mpz (const mpz_t &from_val)
> +  {
> +    mpz_init (val);
> +    mpz_set (val, from_val);
> +  }
> +
> +  gdb_mpz (const gdb_mpz &from)
> +  {
> +    mpz_init (val);
> +    mpz_set (val, from.val);
> +  }
> +
> +  /* Initialize using the given integral value.
> +
> +     The main advantage of this method is that it handles both signed
> +     and unsigned types, with no size restriction.  */
> +  template<typename T, typename = gdb::Requires<std::is_integral<T>>>
> +  explicit gdb_mpz (T src)
> +  {
> +    mpz_init (val);
> +    set (src);
> +  }
> +
> +  explicit gdb_mpz (gdb_mpz &&from)
> +  {
> +    mpz_init (val);
> +    mpz_swap (val, from.val);
> +  }
> +
> +
> +  gdb_mpz &operator= (const gdb_mpz &from)
> +  {
> +    mpz_set (val, from.val);
> +    return *this;
> +  }
> +
> +  gdb_mpz &operator== (gdb_mpz &&other)
> +  {
> +    mpz_swap (val, other.val);
> +    return *this;
> +  }

Is this meant to be "operator="?

> +
> +  template<typename T, typename = gdb::Requires<std::is_integral<T>>>
> +  gdb_mpz &operator= (T src)
> +  {
> +    set (src);
> +    return *this;
> +  }
> +
> +  /* Convert VAL to an integer of the given type.
> +
> +     The return type can signed or unsigned, with no size restriction.  */
> +  template<typename T> T as_integer () const;
> +
> +  /* Set VAL by importing the number stored in the byte buffer (BUF),
> +     given its size (LEN) and BYTE_ORDER.
> +
> +     UNSIGNED_P indicates whether the number has an unsigned type.  */
> +  void read (const gdb_byte *buf, int len, enum bfd_endian byte_order,
> +	     bool unsigned_p);
> +
> +  /* Write VAL into BUF as a LEN-bytes number with the given BYTE_ORDER.
> +
> +     UNSIGNED_P indicates whether the number has an unsigned type.  */
> +  void write (gdb_byte *buf, int len, enum bfd_endian byte_order,
> +	      bool unsigned_p) const;

These two would be good candidates for gdb::array_view.

> +
> +  /* Return a string containing VAL.  */
> +  gdb::unique_xmalloc_ptr<char> str () const
> +  { return gmp_string_asprintf ("%Zd", val); }
> +
> +  /* The destructor.  */
> +  ~gdb_mpz () { mpz_clear (val); }
> +
> +private:
> +
> +  /* Helper template for constructor and operator=.  */
> +  template<typename T> void set (T src);
> +};
> +
> +/* A class to make it easier to use GMP's mpq_t values within GDB.  */
> +
> +struct gdb_mpq
> +{
> +  mpq_t val;
> +
> +  /* Constructors.  */
> +  gdb_mpq () { mpq_init (val); }
> +
> +  explicit gdb_mpq (const mpq_t &from_val)
> +  {
> +    mpq_init (val);
> +    mpq_set (val, from_val);
> +  }
> +
> +  gdb_mpq (const gdb_mpq &from)
> +  {
> +    mpq_init (val);
> +    mpq_set (val, from.val);
> +  }
> +
> +  explicit gdb_mpq (gdb_mpq &&from)
> +  {
> +    mpq_init (val);
> +    mpq_swap (val, from.val);
> +  }
> +
> +  /* Copy assignment operator.  */
> +  gdb_mpq &operator= (const gdb_mpq &from)
> +  {
> +    mpq_set (val, from.val);
> +    return *this;
> +  }
> +
> +  gdb_mpq &operator= (gdb_mpq &&from)
> +  {
> +    mpq_swap (val, from.val);
> +    return *this;
> +  }
> +
> +  /* Return a string representing VAL as "<numerator> / <denominator>".  */
> +  gdb::unique_xmalloc_ptr<char> str () const
> +  { return gmp_string_asprintf ("%Qd", val); }
> +
> +  /* Return VAL rounded to the nearest integer.  */
> +  gdb_mpz get_rounded () const;
> +
> +  /* Set VAL from the contents of the given buffer (BUF), which
> +     contains the unscaled value of a fixed point type object
> +     with the given size (LEN) and byte order (BYTE_ORDER).
> +
> +     UNSIGNED_P indicates whether the number has an unsigned type.
> +     SCALING_FACTOR is the scaling factor to apply after having
> +     read the unscaled value from our buffer.  */
> +  void read_fixed_point (const gdb_byte *buf, int len,
> +			 enum bfd_endian byte_order, bool unsigned_p,
> +			 const gdb_mpq &scaling_factor);
> +
> +  /* Write VAL into BUF as a LEN-bytes fixed point value following
> +     the given BYTE_ORDER.
> +
> +     UNSIGNED_P indicates whether the number has an unsigned type.
> +     SCALING_FACTOR is the scaling factor to apply before writing
> +     the unscaled value to our buffer.  */
> +  void write_fixed_point (gdb_byte *buf, int len,
> +			  enum bfd_endian byte_order, bool unsigned_p,
> +			  const gdb_mpq &scaling_factor) const;
> +
> +  /* The destructor.  */
> +  ~gdb_mpq () { mpq_clear (val); }
> +};
> +
> +/* A class to make it easier to use GMP's mpz_t values within GDB.

mpz_t -> mpf_t

> +extern void _initialize_gmp_utils_selftests ();

You can drop "extern".

Simon