From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55587 invoked by alias); 18 Apr 2017 14:54:14 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 55573 invoked by uid 89); 18 Apr 2017 14:54:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=substance, onlinedocs, vladimir, actively X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Apr 2017 14:54:11 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6428178802; Tue, 18 Apr 2017 14:54:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6428178802 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6428178802 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 919747815E; Tue, 18 Apr 2017 14:54:11 +0000 (UTC) Subject: Re: [PATCH] gdb passes and returns incorrect values when dealing with small array on Sparc To: vladimir.mezentsev@oracle.com, gdb-patches@sourceware.org References: <1490804220-92717-1-git-send-email-vladimir.mezentsev@oracle.com> From: Pedro Alves Message-ID: Date: Tue, 18 Apr 2017 14:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1490804220-92717-1-git-send-email-vladimir.mezentsev@oracle.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00508.txt.bz2 Hi Vladimir, Sorry for the delay. I think everyone was hoping that someone that actually groks SPARC stuff would take a look at this. I don't think we have any maintainer currently actively caring for SPARC in particular. Maybe you guys (Oracle folks) could help review each others' patches? I think that'd help get these patches in quicker. I'll comment below on formatting / convention / procedural stuff, and assume the patch's actual substance is correct. On 03/29/2017 05:17 PM, vladimir.mezentsev@oracle.com wrote: > From: Vladimir Mezentsev > > gdb has a special type (TYPE_CODE_ARRAY) to support the gcc extension > (https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html). > TYPE_CODE_ARRAY is handled incorrectly for both (32- and 64-bit) modes on Sparc machines. > > Tested on sparc64-linux-gnu and sparc-solaris (32- and 64-bit mode). > No regressions. Note there should be no leading-tab indentation in the body of the commit log. Please make sure this is reindented on the left. > > 2017-03-27 Vladimir Mezentsev Two spaces before and after your name. > > * gdb/sparc-tdep.c (sparc_structure_return_p): New function. > * gdb/sparc-tdep.c (sparc_arg_on_registers_p): Likewise. > * gdb/sparc-tdep.c (sparc32_store_arguments): Use the new functions. > * gdb/sparc64-tdep.c: (sparc64_16_byte_align_p): Handle TYPE_CODE_ARRAY > * gdb/sparc64-tdep.c: (sparc64_store_floating_fields): Likewise > * gdb/sparc64-tdep.c: (sparc64_extract_floating_fields): Likewise Entries are relative to the closest ChangeLog file, which is gdb/ChangeLog in this case. File names should be not repeated, and there should be a period after "Likewise" (though you merge those entries). There should be no ":" between the file name and function name. Here's how I'd fix it: gdb/ChangeLog: 2017-03-27 Vladimir Mezentsev * sparc-tdep.c (sparc_structure_return_p) (sparc_arg_on_registers_p): New functions. (sparc32_store_arguments): Use them. * sparc64-tdep.c (sparc64_16_byte_align_p) (sparc64_store_floating_fields, sparc64_extract_floating_fields): Handle TYPE_CODE_ARRAY. You can find these details and more at . > --- > gdb/sparc-tdep.c | 68 +++++++++++++++++++++++++++++++++++++++------------- > gdb/sparc64-tdep.c | 42 ++++++++++++++++++++++++++++++- > 2 files changed, 91 insertions(+), 19 deletions(-) > > diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c > index d346aec..07808f3 100644 > --- a/gdb/sparc-tdep.c > +++ b/gdb/sparc-tdep.c > @@ -297,6 +297,39 @@ sparc_structure_or_union_p (const struct type *type) > return 0; > } > > +static int > +sparc_structure_return_p (const struct type *type) All functions should have an intro comment. While at it, please let's use bool/true/false in new code. > +{ > + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8)) GDB/GNU's coding standard avoids redundant parenthesis: if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_LENGTH (type) <= 8) etc. throughout the patch. > + { > + struct type *t = check_typedef (TYPE_TARGET_TYPE (type)); > + > + if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8)) ... here ... > + return 1; > + return 0; > + } > + if (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16)) ... here ... > + return 1; > + return sparc_structure_or_union_p (type); > +} > + > +static int > +sparc_arg_on_registers_p (const struct type *type) > +{ > + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (TYPE_LENGTH (type) <= 8)) ... here ... > + { > + struct type *t = check_typedef (TYPE_TARGET_TYPE (type)); > + > + if (sparc_floating_p (t) && (TYPE_LENGTH (t) == 8)) ... here ... > + return 0; > + return 1; > + } > + if (sparc_structure_or_union_p (type) || sparc_complex_floating_p (type) > + || (sparc_floating_p (type) && (TYPE_LENGTH (type) == 16))) ... here ... > + return 0; > + return 1; > +} > + > /* Register information. */ > #define SPARC32_FPU_REGISTERS \ > "f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7", \ > @@ -569,9 +602,7 @@ sparc32_store_arguments (struct regcache *regcache, int nargs, > struct type *type = value_type (args[i]); > int len = TYPE_LENGTH (type); > > - if (sparc_structure_or_union_p (type) > - || (sparc_floating_p (type) && len == 16) > - || sparc_complex_floating_p (type)) > + if (!sparc_arg_on_registers_p (type)) > { > /* Structure, Union and Quad-Precision Arguments. */ > sp -= len; > @@ -593,11 +624,8 @@ sparc32_store_arguments (struct regcache *regcache, int nargs, > else > { > /* Integral and pointer arguments. */ > - gdb_assert (sparc_integral_or_pointer_p (type)); > - > - if (len < 4) > - args[i] = value_cast (builtin_type (gdbarch)->builtin_int32, > - args[i]); > + gdb_assert (sparc_integral_or_pointer_p (type) || || goes on the next line. > + ((TYPE_CODE (type) == TYPE_CODE_ARRAY) && (len <= 8))); Redundant parens. > num_elements += ((len + 3) / 4); > } > } > @@ -619,6 +647,15 @@ sparc32_store_arguments (struct regcache *regcache, int nargs, > const bfd_byte *valbuf = value_contents (args[i]); > struct type *type = value_type (args[i]); > int len = TYPE_LENGTH (type); > + gdb_byte buf[4]; > + > + if (len < 4) > + { > + memset (buf, 0, 4 - len); > + memcpy (buf + 4 - len, valbuf, len); > + valbuf = buf; > + len = 4; > + } > > gdb_assert (len == 4 || len == 8); > > @@ -1344,10 +1381,10 @@ sparc32_extract_return_value (struct type *type, struct regcache *regcache, > int len = TYPE_LENGTH (type); > gdb_byte buf[32]; > > - gdb_assert (!sparc_structure_or_union_p (type)); > - gdb_assert (!(sparc_floating_p (type) && len == 16)); > + gdb_assert (!sparc_structure_return_p (type)); > > - if (sparc_floating_p (type) || sparc_complex_floating_p (type)) > + if (sparc_floating_p (type) || sparc_complex_floating_p (type) || > + (TYPE_CODE (type) == TYPE_CODE_ARRAY)) Redundant parens. "||" goes on next line. When we have several conditions that don't fit on one line, I think it reads better to then put each on its own line: if (sparc_floating_p (type) || sparc_complex_floating_p (type) || TYPE_CODE (type) == TYPE_CODE_ARRAY) > { > /* Floating return values. */ > regcache_cooked_read (regcache, SPARC_F0_REGNUM, buf); > @@ -1396,11 +1433,9 @@ sparc32_store_return_value (struct type *type, struct regcache *regcache, > const gdb_byte *valbuf) > { > int len = TYPE_LENGTH (type); > - gdb_byte buf[8]; > + gdb_byte buf[32]; Is it obvious for someone looking at this function what this "32" is and whether it is big enough? > > - gdb_assert (!sparc_structure_or_union_p (type)); > - gdb_assert (!(sparc_floating_p (type) && len == 16)); > - gdb_assert (len <= 8); > + gdb_assert (!sparc_structure_return_p (type)); > > if (sparc_floating_p (type) || sparc_complex_floating_p (type)) > { > @@ -1456,8 +1491,7 @@ sparc32_return_value (struct gdbarch *gdbarch, struct value *function, > guarantees that we can always find the return value, not just > before the function returns. */ > > - if (sparc_structure_or_union_p (type) > - || (sparc_floating_p (type) && TYPE_LENGTH (type) == 16)) > + if (sparc_structure_return_p (type)) > { > ULONGEST sp; > CORE_ADDR addr; > diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c > index bf0da18..0b1a0ee 100644 > --- a/gdb/sparc64-tdep.c > +++ b/gdb/sparc64-tdep.c > @@ -669,6 +669,12 @@ static const struct frame_base sparc64_frame_base = > static int > sparc64_16_byte_align_p (struct type *type) > { > + if ((TYPE_CODE (type) == TYPE_CODE_ARRAY)) { Redundant parens. { does on the next line. > + struct type *t = check_typedef (TYPE_TARGET_TYPE (type)); > + > + if (sparc64_floating_p (t)) > + return 1; > + } > if (sparc64_floating_p (type) && TYPE_LENGTH (type) == 16) > return 1; > > @@ -703,7 +709,23 @@ sparc64_store_floating_fields (struct regcache *regcache, struct type *type, > > gdb_assert (element < 16); > > - if (sparc64_floating_p (type) > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + gdb_byte buf[8]; > + int regnum = SPARC_F0_REGNUM + element * 2 + bitpos / 32; > + > + valbuf += (bitpos / 8); Redundant parens. > + if (len < 8) > + { > + memset (buf, 0, 8 - len); > + memcpy (buf + 8 - len, valbuf, len); > + valbuf = buf; > + len = 8; > + } > + for (int n = 0; n < ((len + 3) / 4); n++) Ditto. > + regcache_cooked_write (regcache, regnum + n, valbuf + n * 4); > + } > + else if (sparc64_floating_p (type) > || (sparc64_complex_floating_p (type) && len <= 16)) Ditto. > { > int regnum; > @@ -776,7 +798,23 @@ sparc64_extract_floating_fields (struct regcache *regcache, struct type *type, > { > struct gdbarch *gdbarch = get_regcache_arch (regcache); > > - if (sparc64_floating_p (type)) > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + int len = TYPE_LENGTH (type); > + int regnum = SPARC_F0_REGNUM + (bitpos / 32); Spurious space after " = ". Redundant parens. > + > + valbuf += (bitpos / 8); > + if (len < 4) > + { > + gdb_byte buf[4]; > + regcache_cooked_read (regcache, regnum, buf); > + memcpy (valbuf, buf + 4 - len, len); > + } > + else > + for (int i = 0; i < ((len + 3) / 4); i++) > + regcache_cooked_read (regcache, regnum + i, valbuf + i * 4); Redundant parens multiple times. > + } > + else if (sparc64_floating_p (type)) > { > int len = TYPE_LENGTH (type); > int regnum; > Please resubmit a v2 patch with these issues addressed. Thanks, Pedro Alves