From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18736 invoked by alias); 8 Nov 2018 14:44:32 -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 18726 invoked by uid 89); 8 Nov 2018 14:44:31 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 Nov 2018 14:44:29 +0000 Received: by mail-wm1-f65.google.com with SMTP id s10-v6so1526177wmc.5 for ; Thu, 08 Nov 2018 06:44:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=uqqyPvV1aNYDi40rrACtTox4zMErwwnxK5bkDYkgR0w=; b=IbH/iaPjZxjzQLRpeOsPLfA9pOrX0iiHe42iFxqDP0ivRgzQRUW3AIPT7Adh3/rOh1 M3iMfv7aLXxjYdmAnt2GAGbnCoYc2bpaCTcnrhHBFyYEbF312731u34jj/8YXxck8Xc0 +DcWE6PlX9v0Qv0RxG8On0uwH4mxL3pVG8jWlF5yGCe9b9dcRrFv5zE4GmIJxG6wGOFW Yt4BP4N7Mh2CuKlbRxbCsBXDKCT6auJE/KvQqlukJZhTdszHKQSQdQ6XugEYprfsmkQn J+xQLcI47q2HzdDdXmvvMat3dLYRM7GpdF9KCT3MZL8tpetwCl27eLAZyR1gwB6kjXc9 eVNw== Return-Path: Received: from localhost ([2a02:390:741d:1:3665:d267:b319:d766]) by smtp.gmail.com with ESMTPSA id a126-v6sm3952719wmh.4.2018.11.08.06.44.26 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 08 Nov 2018 06:44:26 -0800 (PST) Date: Thu, 08 Nov 2018 14:44:00 -0000 From: Andrew Burgess To: Jim Wilson Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3] RISC-V: gdb.base/gnu_vector fixes. Message-ID: <20181108144425.GZ16539@embecosm.com> References: <20181106214446.22325-1-jimw@sifive.com> <20181108143411.GX16539@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181108143411.GX16539@embecosm.com> X-Fortune: There is no substitute for good manners, except, perhaps, fast reflexes. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00119.txt.bz2 * Andrew Burgess [2018-11-08 14:34:11 +0000]: > * Jim Wilson [2018-11-06 13:44:46 -0800]: > > > Unnamed arguments with 2*XLEN alignment are passed in aligned register pairs. > > > > gdb/ > > * riscv-tdep.c (struct riscv_arg_info): New field is_unnamed. > > (riscv_call_arg_scalar_int): If unnamed arg with twice xlen alignment, > > then increment next_regnum if odd. > > (riscv_arg_location): New arg is_unnamed. Set ainfo->is_unnamed. > > (riscv_push_dummy_call): New local ftype. Call check_typedef to set > > function type. Pass new arg to riscv_arg_location based on function > > type. > > (riscv_return_value): Pass new arg to riscv_arg_location. > > Could you change the titles for the 3 commits in this series please to > make them more descriptive of the fix in each commit. This also > avoids having 3 commits in a row with the same title line, which can > look like a mistake in 'git log --format=oneline'. Also can you make sure the commit message for each patch includes details about which tests you expect to go from fail to pass, and on which RISC-V variants you expect to see improvements. Thanks, Andrew > > > > > > > --- > > gdb/riscv-tdep.c | 22 +++++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > > index 3d4f7e3dcc..93310c329f 100644 > > --- a/gdb/riscv-tdep.c > > +++ b/gdb/riscv-tdep.c > > @@ -1737,6 +1737,9 @@ struct riscv_arg_info > > then this offset will be set to 0. */ > > int c_offset; > > } argloc[2]; > > + > > + /* TRUE if this is an unnamed argument. */ > > + bool is_unnamed; > > }; > > > > /* Information about a set of registers being used for passing arguments as > > @@ -1932,6 +1935,12 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo, > > { > > int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length; > > > > + /* Unnamed arguments in registers that require 2*XLEN alignment are > > + passed in an aligned register pair. */ > > + if (ainfo->is_unnamed && (ainfo->align == cinfo->xlen * 2) > > + && cinfo->int_regs.next_regnum & 1) > > + cinfo->int_regs.next_regnum++; > > + > > if (!riscv_assign_reg_location (&ainfo->argloc[0], > > &cinfo->int_regs, len, 0)) > > riscv_assign_stack_location (&ainfo->argloc[0], > > @@ -2222,11 +2231,12 @@ static void > > riscv_arg_location (struct gdbarch *gdbarch, > > struct riscv_arg_info *ainfo, > > struct riscv_call_info *cinfo, > > - struct type *type) > > + struct type *type, bool is_unnamed) > > Could you update the functions header comment to explain what impact > IS_UNNAMED will have on the behaviour of the function. > > > { > > ainfo->type = type; > > ainfo->length = TYPE_LENGTH (ainfo->type); > > ainfo->align = riscv_type_alignment (ainfo->type); > > + ainfo->is_unnamed = is_unnamed; > > ainfo->contents = nullptr; > > > > switch (TYPE_CODE (ainfo->type)) > > @@ -2375,6 +2385,11 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, > > > > CORE_ADDR osp = sp; > > > > + struct type *ftype = check_typedef (value_type (function)); > > + > > + if (TYPE_CODE (ftype) == TYPE_CODE_PTR) > > + ftype = check_typedef (TYPE_TARGET_TYPE (ftype)); > > + > > /* We'll use register $a0 if we're returning a struct. */ > > if (struct_return) > > ++call_info.int_regs.next_regnum; > > @@ -2388,7 +2403,8 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, > > arg_value = args[i]; > > arg_type = check_typedef (value_type (arg_value)); > > > > - riscv_arg_location (gdbarch, info, &call_info, arg_type); > > + riscv_arg_location (gdbarch, info, &call_info, arg_type, > > + TYPE_VARARGS (ftype) && i >= TYPE_NFIELDS (ftype)); > > > > if (info->type != arg_type) > > arg_value = value_cast (info->type, arg_value); > > @@ -2565,7 +2581,7 @@ riscv_return_value (struct gdbarch *gdbarch, > > struct type *arg_type; > > > > arg_type = check_typedef (type); > > - riscv_arg_location (gdbarch, &info, &call_info, arg_type); > > + riscv_arg_location (gdbarch, &info, &call_info, arg_type, false); > > > > if (riscv_debug_infcall > 0) > > { > > -- > > 2.17.1 > > > > Otherwise looks fine. > > Thanks, > Andrew