From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id gEDCLOH0ql8UFgAAWB0awg (envelope-from ) for ; Tue, 10 Nov 2020 15:15:29 -0500 Received: by simark.ca (Postfix, from userid 112) id B15B71F08B; Tue, 10 Nov 2020 15:15:29 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 7EEA61E58F for ; Tue, 10 Nov 2020 15:15:28 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 47D9A398680E; Tue, 10 Nov 2020 20:15:28 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 3A39B3986807 for ; Tue, 10 Nov 2020 20:15:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3A39B3986807 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D9CB11E58F; Tue, 10 Nov 2020 15:15:24 -0500 (EST) Subject: Re: [PATCH 3/9] gmp-utils: New API to simply use of GMP's integer/rational/float objects To: Joel Brobecker , gdb-patches@sourceware.org References: <1604817017-25807-1-git-send-email-brobecker@adacore.com> <1604817017-25807-4-git-send-email-brobecker@adacore.com> From: Simon Marchi Message-ID: <2966f3cf-955c-1849-e7ab-f01843cb7a33@simark.ca> Date: Tue, 10 Nov 2020 15:15:24 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <1604817017-25807-4-git-send-email-brobecker@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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 . */ > + > +#ifndef GMP_UTILS_H > +#define GMP_UTILS_H > + > +#include "defs.h" > + > +/* Include and ahead of , so as to get > + access to GMP's various formatting functions. */ > +#include > +#include > +#include > +#include "gdbsupport/traits.h" > + > +/* Same as gmp_asprintf, but returning a convenient wrapper type. */ > + > +gdb::unique_xmalloc_ptr 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>> > + 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>> > + 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 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 str () const > + { return gmp_string_asprintf ("%Zd", val); } > + > + /* The destructor. */ > + ~gdb_mpz () { mpz_clear (val); } > + > +private: > + > + /* Helper template for constructor and operator=. */ > + template 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 " / ". */ > + gdb::unique_xmalloc_ptr 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