From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id B766E385DC3E for ; Wed, 8 Apr 2020 04:11:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B766E385DC3E 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] (unknown [192.222.164.54]) (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 AE62B1E60C; Wed, 8 Apr 2020 00:11:10 -0400 (EDT) Subject: Re: [PATCH] Add SVR4 psABI specific parser for AUXV entries To: Kamil Rytarowski , gdb-patches@sourceware.org Cc: tom@tromey.com References: <20200408021426.28406-1-n54@gmx.com> From: Simon Marchi Message-ID: <5b988c52-2bd0-6af9-5c6e-147c639d52a5@simark.ca> Date: Wed, 8 Apr 2020 00:11:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200408021426.28406-1-n54@gmx.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-24.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 08 Apr 2020 04:11:13 -0000 On 2020-04-07 10:14 p.m., Kamil Rytarowski wrote: > NetBSD and OpenBSD always use an int to store the type as > defined in the SVR4 psABI specifications rather than long > as assumed by the default parser. > > Define svr4_auxv_parse() that shares code with default_auxv_parse(). > > Remove obsd_auxv_parse() and switch OpenBSD to svr4_auxv_parse(). > Remove not fully accurate comment from obsd-tdep.c. > > Use svr4_auxv_parse() on NetBSD. > > gdb/ChangeLog: > > * auxv.h (svr4_auxv_parse): New. > * auxv.c (default_auxv_parse): Split into default_auxv_parse > and generic_auxv_parse. > (svr4_auxv_parse): Add. > * obsd-tdep.c: Include "auxv.h". > (obsd_auxv_parse): Remove. > (obsd_init_abi): Remove comment. > (obsd_init_abi): Change set_gdbarch_auxv_parse passed argument > from `obsd_auxv_parse' to `svr4_auxv_parse'. > * nbsd-tdep.c: Include "auxv.h". > (nbsd_init_abi): Call set_gdbarch_auxv_parse. Thanks, just a few minor comments. > diff --git a/gdb/auxv.c b/gdb/auxv.c > index c13d7a22eb9..240f32236b3 100644 > --- a/gdb/auxv.c > +++ b/gdb/auxv.c > @@ -248,34 +248,64 @@ memory_xfer_auxv (struct target_ops *ops, > return procfs_xfer_auxv (readbuf, writebuf, offset, len, xfered_len); > } > > -/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR. > - Return 0 if *READPTR is already at the end of the buffer. > - Return -1 if there is insufficient buffer for a whole entry. > - Return 1 if an entry was read into *TYPEP and *VALP. */ > -int > -default_auxv_parse (struct target_ops *ops, gdb_byte **readptr, > - gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp) > +/* Generic auxv_parse body shared with default_auxv_parse() and > + svr4_auxv_parse(). */ Instead of saying that it's used by this and that function (which is doomed to become obsolete very easily anyway), say what is the particularity of this function, compared to other auxv_parse functions: it takes the size of the auxv type field as a parameter. > + > +static int > +generic_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr, > + gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp, > + int sizeof_auxv_type) > { > - const int sizeof_auxv_field = gdbarch_ptr_bit (target_gdbarch ()) > - / TARGET_CHAR_BIT; > - const enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ()); > + struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr; > + const int sizeof_auxv_val = TYPE_LENGTH (ptr_type); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > gdb_byte *ptr = *readptr; > > if (endptr == ptr) > return 0; > > - if (endptr - ptr < sizeof_auxv_field * 2) > + if (endptr - ptr < 2 * sizeof_auxv_val) > return -1; > > - *typep = extract_unsigned_integer (ptr, sizeof_auxv_field, byte_order); > - ptr += sizeof_auxv_field; > - *valp = extract_unsigned_integer (ptr, sizeof_auxv_field, byte_order); > - ptr += sizeof_auxv_field; > + *typep = extract_unsigned_integer (ptr, sizeof_auxv_type, byte_order); > + ptr += sizeof_auxv_val; /* Alignment. */ Instead of just "Alignment", please add a comment explaining that even if the auxv type takes less space than an auxv value, there is padding after the type such that the value is aligned on a multiple of its size (and this is why we advance by `sizeof_auxv_val` and not `sizeof_auxv_type`. I think that can be surprising for somebody who does not know this fact. > + *valp = extract_unsigned_integer (ptr, sizeof_auxv_val, byte_order); > + ptr += sizeof_auxv_val; > > *readptr = ptr; > return 1; > } > > +/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR. > + Return 0 if *READPTR is already at the end of the buffer. > + Return -1 if there is insufficient buffer for a whole entry. > + Return 1 if an entry was read into *TYPEP and *VALP. */ Since this is the implementation of an exported function, replace the comment here with: /* See auxv.h. */ and document in the header file. I think the documentation you have put in the header file for svr4_auxv_parse is good, it explains what the function does and how it's different from default_auxv_parse. I don't think it's particularly useful to describe all parameters here, since it's a function that implements an interface. The parameters should be documented where the interface is defined. > +int > +default_auxv_parse (struct target_ops *ops, gdb_byte **readptr, > + gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp) This last line is not properly aligned on the parenthesis. > +{ > + struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr; > + const int sizeof_auxv_type = TYPE_LENGTH (ptr_type); > + > + return generic_auxv_parse (target_gdbarch (), readptr, endptr, typep, valp, > + sizeof_auxv_type); > +} > + > +/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR. > + Return 0 if *READPTR is already at the end of the buffer. > + Return -1 if there is insufficient buffer for a whole entry. > + Return 1 if an entry was read into *TYPEP and *VALP. */ Same comment about the comment. > +int > +svr4_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr, > + gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp) This last line is not properly aligned on the parenthesis. Simon