From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98515 invoked by alias); 25 May 2017 10:46:28 -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 98477 invoked by uid 89); 25 May 2017 10:46:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LOTSOFHASH,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f41.google.com Received: from mail-wm0-f41.google.com (HELO mail-wm0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 May 2017 10:46:24 +0000 Received: by mail-wm0-f41.google.com with SMTP id 7so86305281wmo.1 for ; Thu, 25 May 2017 03:46:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=3mQOwkqWkxufodDIquXlZKJYvRvv7C7tD5SzUG5kFcw=; b=lYID5+PLN6xq8EykNnCqHMh4vJmjZKblEP0Bo2qGx6Zx6qjwxf3Na43SY3lJ4lUKya oz16wQLXER4NEHKAEGBtw4+cL3+rxE/IZHl0bTKTxQ1mkDNKojjtllWR4v/6o+WoVuuj vzV4jizFnU00YCF+rK/BxyrZSPVkl5uhMcVcHV4T+MdxqQTMsVb804RzwAeoVK3or3MN o4Fs2KnaJBkAA1fMJg0qOV+XgtlDIUgXf3vQnFVTFCJ+r9cL2xTT6sqX36sW1ZxN2ffK MAtzGx4eJLwfJZJatqroUJnnBXfuPyvysY9aGK5BsP3Yz/hw/U2PGgNMkdZTzDj9KooC HPMQ== X-Gm-Message-State: AODbwcCQPVgItfVRauaJF7oOx9SDHLxh+qB/ziY6uDO+BryMVEzX4isq 6DYrkqmhtXcfAIpD X-Received: by 10.28.136.83 with SMTP id k80mr8798336wmd.38.1495709185995; Thu, 25 May 2017 03:46:25 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id v22sm4204045wrd.38.2017.05.25.03.46.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 25 May 2017 03:46:25 -0700 (PDT) Subject: Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4) To: Alan Hayward References: <3C00280E-37C9-4C0A-9DA6-F3B9DB1A6E8F@arm.com> <86y3v7uf9j.fsf@gmail.com> <806B436F-EFA1-4200-AC54-9036D166C9B9@arm.com> <867f1m8nhm.fsf@gmail.com> <8637bx9jsw.fsf@gmail.com> <78A7E8EA-7203-44DF-B7FD-63E75A5ECEF5@arm.com> <540372d8-efc3-f842-5cac-cd813bacc3f5@redhat.com> <4F90CD36-759D-4BDA-BFEC-8DD86F44A0B7@arm.com> <40597975-9458-e9af-8915-9d303bb1ed98@redhat.com> <5A105765-C70D-413C-BB35-50BAA5FD5865@arm.com> Cc: Yao Qi , "gdb-patches@sourceware.org" , nd From: Pedro Alves Message-ID: <73b5b4f8-065b-7102-a9d8-0b909b1eb124@redhat.com> Date: Thu, 25 May 2017 10:46: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: <5A105765-C70D-413C-BB35-50BAA5FD5865@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00537.txt.bz2 On 05/24/2017 08:45 PM, Alan Hayward wrote: > Added copy_integer_to_size, and removed the templates. > > Manually tested of copy_integer_to_size to make sure the signs and > endian parts all work. Those manual tests would have been perfect candidates for some unit tests. All you'd need to do is add this at the bottom of gdb/findvar.c: #if GDB_SELF_TEST namespace selftests { namespace findvar_tests { static void run_test () { // Here, exercise the various code paths of copy_integer_to_size, // calling SELF_CHECK. } } // namespace findvar_test } // namespace selftests #endif void _initialize_findvar (void) { #if GDB_SELF_TEST register_self_test (selftests::findvar_tests::run_test); #endif } (and include selftest.h at the top of the file). You'd run those tests with: $ make check TESTS="gdb.gdb/unittest.exp" Or (my preferred when hacking): $ gdb --batch -q -ex "maint selftest" > Tested on a --enable-targets=all build using make check with board files > unix and native-gdbserver. > I do not have a MIPS machine to test on. > Ok to commit? On 05/24/2017 08:45 PM, Alan Hayward wrote: > > > 2017-05-24 Alan Hayward > > * gdb/defs.h (copy_integer_to_size): New declaration. > * gdb/findvar.c (extract_signed_integer): Removed function. > (extract_unsigned_integer): Likewise. > (store_signed_integer): Removed function. > (store_unsigned_integer): Likewise. > * mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer. > (mips_fbsd_collect_reg): Use templated raw_collect_integer. > * mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer. > (mips64_fill_gregset): Use raw_collect_integer. > (mips64_fill_fpregset): Use raw_supply_integer. > * gdb/regcache.c (regcache::raw_supply_integer): New function. > (regcache::raw_collect_integer): Likewise > * gdb/regcache.h (regcache::raw_supply): New declaration. > (regcache::raw_collect): Likewise There are stale entries above. Also, drop "gdb/" prefix, and add missing periods after "Likewise". > > > diff --git a/gdb/defs.h b/gdb/defs.h > index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644 > --- a/gdb/defs.h > +++ b/gdb/defs.h > @@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int, > extern void store_typed_address (gdb_byte *buf, struct type *type, > CORE_ADDR addr); > > - > > +extern void copy_integer_to_size (gdb_byte *dest, int dest_size, > + const gdb_byte *source, int source_size, > + bool is_signed, enum bfd_endian byte_order); > + > /* From valops.c */ > > extern int watchdog; > diff --git a/gdb/findvar.c b/gdb/findvar.c > index ed4d5c1266c9de069981b306bc8229ae5fb02350..5a82e493f9ca6d9337a22defc4377235f36acba8 100644 > --- a/gdb/findvar.c > +++ b/gdb/findvar.c > @@ -249,7 +249,47 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr) > gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr); > } > > +/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE > + bytes. If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most > + significant bytes. If SOURCE_SIZE is less than DEST_SIZE then either sign > + or zero extended according to IS_SIGNED. Values are stored in memory with > + endianess BYTE_ORDER. */ > > +void > +copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source, > + int source_size, bool is_signed, > + enum bfd_endian byte_order) > +{ > + signed int size_diff = dest_size - source_size; > + > + /* Copy across everything from SOURCE that can fit into DEST. */ > + > + if (byte_order == BFD_ENDIAN_BIG && size_diff > 0) > + memcpy (dest + size_diff, source, source_size); > + else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0) > + memcpy (dest, source - size_diff, dest_size); > + else > + memcpy (dest, source, std::min (source_size, dest_size)); > + > + /* Fill the remaining space in DEST by either zero extending or sign > + extending. */ > + > + if (size_diff > 0) > + { > + char extension = 0; gdb_byte. > + if (is_signed) > + if ((byte_order == BFD_ENDIAN_BIG && source[0] & 0x80) > + || (byte_order != BFD_ENDIAN_BIG > + && source[source_size - 1] & 0x80)) > + extension = 0xff; > + Merge the ifs? Like, e.g.: gdb_byte extension; if (is_signed && ((byte_order == BFD_ENDIAN_BIG && source[0] & 0x80) || (byte_order != BFD_ENDIAN_BIG && source[source_size - 1] & 0x80))) extension = 0xff; else extension = 0; > + /* Extend into MSBs of SOURCE. */ > + if (byte_order == BFD_ENDIAN_BIG) > + memset (dest, extension, size_diff); > + else > + memset (dest + source_size, extension, size_diff); > + } > +} > > /* Return a `value' with the contents of (virtual or cooked) register > REGNUM as found in the specified FRAME. The register's type is > diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c > index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644 > --- a/gdb/mips-fbsd-tdep.c > +++ b/gdb/mips-fbsd-tdep.c > @@ -47,57 +47,24 @@ > 34th is a dummy for padding. */ > #define MIPS_FBSD_NUM_FPREGS 34 > > -/* Supply a single register. If the source register size matches the > - size the regcache expects, this can use regcache_raw_supply(). If > - they are different, this copies the source register into a buffer > - that can be passed to regcache_raw_supply(). */ > +/* Supply a single register. The register size might not match, so use > + regcache->raw_supply_integer (). */ > > static void > mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr, > size_t len) > { > - struct gdbarch *gdbarch = get_regcache_arch (regcache); > - > - if (register_size (gdbarch, regnum) == len) > - regcache_raw_supply (regcache, regnum, addr); > - else > - { > - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > - gdb_byte buf[MAX_REGISTER_SIZE]; > - LONGEST val; > - > - val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order); > - store_signed_integer (buf, register_size (gdbarch, regnum), byte_order, > - val); > - regcache_raw_supply (regcache, regnum, buf); > - } > + regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true); > } Nice! > --- a/gdb/mips-linux-tdep.c > +++ b/gdb/mips-linux-tdep.c > @@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc) > static void > supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr) > { > - struct gdbarch *gdbarch = get_regcache_arch (regcache); > - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > - gdb_byte buf[MAX_REGISTER_SIZE]; > - store_signed_integer (buf, register_size (gdbarch, regnum), byte_order, > - extract_signed_integer ((const gdb_byte *) addr, 4, > - byte_order)); > - regcache_raw_supply (regcache, regnum, buf); > + regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true); > } Nice. :-) [snip several "nice"s] > diff --git a/gdb/regcache.h b/gdb/regcache.h > index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644 > --- a/gdb/regcache.h > +++ b/gdb/regcache.h > @@ -294,8 +294,14 @@ public: > > void raw_collect (int regnum, void *buf) const; > > + void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, > + bool is_signed) const; > + > void raw_supply (int regnum, const void *buf); > > + void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len, > + bool is_signed); > + > void raw_supply_zeroed (int regnum); > > void raw_copy (int regnum, struct regcache *src_regcache); > diff --git a/gdb/regcache.c b/gdb/regcache.c > index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..ec6446b897922a8f9f44bbf94b7f1d198b0a6d4b 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -1189,6 +1189,28 @@ regcache::raw_supply (int regnum, const void *buf) > } > } > > +/* Supply register REGNUM with an integer, whose contents are stored in ADDR, > + with length ADDR_LEN and sign IS_SIGNED, to REGCACHE. */ It'd be good to say something about extending/truncating here, as well as mention that the ADDR contents are assumed to be in target byte order. > + > +void > +regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len, > + bool is_signed) > +{ > +/* Collect register REGNUM from regcache to an integer, whose contents are > + stored in ADDR, with length ADDR_LEN and sign IS_SIGNED. */ Ditto. > + > +void > +regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, > + bool is_signed) const > +{ Otherwise this looks good to me. It'd look great with unit tests. :-) Yao, what do you think? Thanks, Pedro Alves