Message ID | 20181108154344.GA16539@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 5475 invoked by alias); 8 Nov 2018 15:43:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 5463 invoked by uid 89); 8 Nov 2018 15:43:51 -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-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 Nov 2018 15:43:49 +0000 Received: by mail-wr1-f67.google.com with SMTP id j26-v6so21778981wre.1 for <gdb-patches@sourceware.org>; Thu, 08 Nov 2018 07:43:49 -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=24IjEZG2wkMdTBeqgq9CCtVf+yAKX4qZT1TJVD7zJFs=; b=CktWqnWeNaW83m4dVX5KnDFUDEFOkhZrOfHQFfbM/j6A2nrQNuE7bZFu/gh6bIk/RG Kameb0QlQfDctrEM1Q+0wImfBNN3QAfTbGcivRxv9iYieq7+ZZvC75MC9MdpqlN0wH9N xL5RUZgaNHgGyLxhJRMgYr+z9tsX70Lq+OcgwBFXFfihRza90J12xzbbT6BXn2EDxKPK OGoPfd8a3gvOFMT7OgjM7AS8/wzT5+SBAudZgN5CeJogeszrhmfLJrzezQv+d2h7zhB6 CSziA4Rgipm0HmZ/coA2djvaa/c5asd6CTywZNRlruYA/yzbh2xW/48Tb8r/5wXxqM6r whrQ== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost ([2a02:390:741d:1:3665:d267:b319:d766]) by smtp.gmail.com with ESMTPSA id z7-v6sm3834549wrv.21.2018.11.08.07.43.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 08 Nov 2018 07:43:46 -0800 (PST) Date: Thu, 8 Nov 2018 15:43:44 +0000 From: Andrew Burgess <andrew.burgess@embecosm.com> To: Jim Wilson <jimw@sifive.com> Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] RISC-V: gdb.base/gnu_vector fixes. Message-ID: <20181108154344.GA16539@embecosm.com> References: <CAFyWVaYMo6sOfXQqZ3YuRyjSnsfGSB9vZTO43j6hKRva73_TCA@mail.gmail.com> <20181106214403.22192-1-jimw@sifive.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181106214403.22192-1-jimw@sifive.com> X-Fortune: Take it easy, we're in a hurry. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Nov. 8, 2018, 3:43 p.m. UTC
* Jim Wilson <jimw@sifive.com> [2018-11-06 13:44:03 -0800]: > Ensure that stack slots are always the same size as XLEN by rounding up arg > sizes when computing the address of the next stack slot. > > gdb/ > * riscv-tdep.c (riscv_assign_stack_location): New arg slot_align. > Use with align_up when setting arg_offset. > (riscv_call_arg_scalar_int): Call riscv_assign_stack_location with > cinfo->xlen as new arg. > --- > gdb/riscv-tdep.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index db372e2163..ac4f2533f4 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -1868,13 +1868,13 @@ riscv_assign_reg_location (struct riscv_arg_info::location *loc, > static void > riscv_assign_stack_location (struct riscv_arg_info::location *loc, > struct riscv_memory_offsets *memory, > - int length, int align) > + int length, int align, int slot_align) I struggled to understand what the difference between align and slot_align is in this patch, it feels like.... > { > loc->loc_type = riscv_arg_info::location::on_stack; > memory->arg_offset > = align_up (memory->arg_offset, align); > loc->loc_data.offset = memory->arg_offset; > - memory->arg_offset += length; > + memory->arg_offset += align_up (length, slot_align); > loc->c_length = length; > > /* Offset is always 0, either we're the first location part, in which > @@ -1919,7 +1919,7 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo, > cinfo->xlen, 0)) > riscv_assign_stack_location (&ainfo->argloc[1], > &cinfo->memory, cinfo->xlen, > - cinfo->xlen); > + cinfo->xlen, cinfo->xlen); > } > else > { > @@ -1928,7 +1928,8 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo, > if (!riscv_assign_reg_location (&ainfo->argloc[0], > &cinfo->int_regs, len, 0)) > riscv_assign_stack_location (&ainfo->argloc[0], > - &cinfo->memory, len, ainfo->align); > + &cinfo->memory, len, ainfo->align, > + cinfo->xlen); .... you might be better off just passing a better value of align through here. Testing on gdb.base/gnu_vector.exp shows that doing this fixes the same problem that your original patch fixes. My revision is below, but I've only just kicked off a test run, so there might be problems I'm not seeing. Thanks, Andrew > > if (len < ainfo->length) > { > @@ -1937,7 +1938,8 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo, > &cinfo->int_regs, len, > cinfo->xlen)) > riscv_assign_stack_location (&ainfo->argloc[1], > - &cinfo->memory, len, cinfo->xlen); > + &cinfo->memory, len, cinfo->xlen, > + cinfo->xlen); > } > } > } > -- > 2.17.1 > ---
Comments
On Thu, Nov 8, 2018 at 7:43 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > I struggled to understand what the difference between align and > slot_align is in this patch, it feels like.... If you have an argument twice the size of xlen, with alignment twice the size of xlen, then when we pass the first word, align is 2*xlen and slot_align is xlen. > .... you might be better off just passing a better value of align > through here. Testing on gdb.base/gnu_vector.exp shows that doing > this fixes the same problem that your original patch fixes. > - int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length; > + int len = std::min (ainfo->length, cinfo->xlen); > + int align = std::max (ainfo->align, cinfo->xlen); If you do this, and we keep the patch in riscv_assign_stack_location, then we end up allocating 2*xlen bytes for the first xlen bytes of a argument of size 2*len whcih is wrong. I suppose you want to drop the riscv_assign_stack_location change too. That could work, but that leaves arg_offset unaligned after allocating the last argument. Though it looks like the only problem with that is some funny riscv_debug_infcall output. It might say for instance that we have 12 bytes of stack arguments for a 64-bit target, which doesn't make any sense. For a 64-bit target, stack arguments should always be a multiple of 8 bytes. Otherwise, this looks harmless, and this is a problem that we already have, so this isn't anything broken by this patch set. I'll rewrite the patch this way. Jim
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index a0b2d1f5d7..243db12999 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -1951,12 +1951,13 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo, } else { - int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length; + int len = std::min (ainfo->length, cinfo->xlen); + int align = std::max (ainfo->align, cinfo->xlen); if (!riscv_assign_reg_location (&ainfo->argloc[0], &cinfo->int_regs, len, 0)) riscv_assign_stack_location (&ainfo->argloc[0], - &cinfo->memory, len, ainfo->align); + &cinfo->memory, len, align); if (len < ainfo->length) {