From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3549 invoked by alias); 25 Oct 2011 19:03:05 -0000 Received: (qmail 3539 invoked by uid 22791); 25 Oct 2011 19:03:03 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Oct 2011 19:02:49 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RImGt-00008q-UD from pedro_alves@mentor.com ; Tue, 25 Oct 2011 12:02:48 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Tue, 25 Oct 2011 20:02:45 +0100 From: Pedro Alves To: Joel Brobecker Subject: Re: [RFA/RFC] Restore old handling of multi-register variables Date: Tue, 25 Oct 2011 19:34:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.1; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <1317675787-7351-1-git-send-email-brobecker@adacore.com> <201110061854.52856.pedro@codesourcery.com> <20111021233802.GJ19246@adacore.com> In-Reply-To: <20111021233802.GJ19246@adacore.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201110252002.38708.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2011-10/txt/msg00676.txt.bz2 On Saturday 22 October 2011 00:38:02, Joel Brobecker wrote: > Purely in terms of solving the AVR problem, what do you think > of the attached patch? Does it look correct to you? > > I tested it on AVR as well as x86_64-linux, and it seems to work. > > Going beyond that, the new function doesn't provide the extended > interface that you suggest. Doing so seems to be complicating > the implementation more than it's worth. I think that what we > should do, we want to eliminate get_frame_register_value, is look > at the current uses and try to eliminate them. Yeah. > The biggest culprit is the register_to_value gdbarch method (11 > hits). But there is only one location where it's actually called, > and it is.... value_from_register! (just above the section of code > that we're improving). I think it would be easy to change the > profile of that method to return a value. > Then the register_to_value implementations could use get_frame_register_value instead. Yeah, though I suspect that your new read_frame_register_value method may evolve into looking what I suggested anyway. :-) > Other two uses that are different: > - dwarf2loc.c: For DW_OP_piece (read/write) support; Looks fixable. Hmm, I like this. > - spu-tdep.c: We just read the contents of a single register > (get_frame_register_value + extract_unsigned_integer, > so it should be easy to replace them with something else. > diff --git a/gdb/value.c b/gdb/value.c > index 087cdfd..8dc9258 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -3140,6 +3140,35 @@ using_struct_return (struct gdbarch *gdbarch, > != RETURN_VALUE_REGISTER_CONVENTION); > } > > +/* VALUE must be an lval_register value. If regnum is the value's > + associated register number, and len the length of the values type, > + read one or more registers in FRAME, starting with register REGNUM, > + until we've read LEN bytes. */ > + > +void > +read_frame_register_value (struct value *value, struct frame_info *frame) > +{ I think this should be in frame.c instead. value.c is for core struct value stuff. > + int offset = 0; > + int regnum = value->regnum; > + const int len = TYPE_LENGTH (value_type (value)); Do we need check_typedefs here? > + gdb_assert (value->lval == lval_register); > + > + while (offset < len) > + { > + struct value *regval = get_frame_register_value (frame, regnum); > + int reg_len = TYPE_LENGTH (value_type (regval)); > + > + if (offset + reg_len > len) > + reg_len = len - offset; > + value_contents_copy (value, offset, regval, value_offset (regval), > + reg_len); > + > + offset += reg_len; > + regnum++; > + } > +} Otherwise looks good to me. Thanks! -- Pedro Alves