From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32430 invoked by alias); 15 Jul 2013 14:46:17 -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 32420 invoked by uid 89); 15 Jul 2013 14:46:16 -0000 X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,TW_EG autolearn=no version=3.3.1 Received: from Unknown (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 15 Jul 2013 14:46:15 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1Uyk2R-0003EB-4z from Luis_Gustavo@mentor.com ; Mon, 15 Jul 2013 07:46:07 -0700 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by svr-orw-fem-01.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Mon, 15 Jul 2013 07:46:06 -0700 Received: from [172.30.15.113] ([172.30.15.113]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 15 Jul 2013 07:46:06 -0700 Message-ID: <51E40B28.3080605@codesourcery.com> Date: Mon, 15 Jul 2013 14:46:00 -0000 From: Luis Machado Reply-To: lgustavo@codesourcery.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130623 Thunderbird/17.0.7 MIME-Version: 1.0 To: Andreas Arnez CC: gdb-patches@sourceware.org, Ulrich.Weigand@de.ibm.com Subject: Re: [RFA][PATCH v4 1/5] S/390 regmap rework References: <87zju3intq.fsf@br87z6lw.de.ibm.com> <87vc4rinoe.fsf@br87z6lw.de.ibm.com> In-Reply-To: <87vc4rinoe.fsf@br87z6lw.de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00348.txt.bz2 Hi, A few nits and only nits. On 07/03/2013 02:00 PM, Andreas Arnez wrote: > S/390 regmap rework: Represent register maps in a less redundant and > more readable way. Also remove some code repetition. > > /* GNU/Linux target descriptions. */ > extern struct target_desc *tdesc_s390_linux32; > Index: gdb/gdb/s390-nat.c > =================================================================== > --- gdb.orig/gdb/s390-nat.c > +++ gdb/gdb/s390-nat.c > @@ -63,139 +63,129 @@ static int have_regset_system_call = 0; > > #define regmap_fpregset s390_regmap_fpregset > > -/* When debugging a 32-bit executable running under a 64-bit kernel, > - we have to fix up the 64-bit registers we get from the kernel > - to make them look like 32-bit registers. */ > +static void > +s390_native_supply (struct regcache *regcache, const short *map, > + const gdb_byte *regp) > +{ > + for (; map[0] >= 0; map += 2) > + regcache_raw_supply (regcache, map[1], regp + map[0]); > +} > > static void > -s390_native_supply (struct regcache *regcache, int regno, > - const gdb_byte *regp, int *regmap) > +s390_native_collect (const struct regcache *regcache, const short *map, > + int regno, gdb_byte *regp) > { > - int offset = regmap[regno]; > + for (; map[0] >= 0; map += 2) > + if (regno == -1 || regno == map[1]) > + regcache_raw_collect (regcache, map[1], regp + map[0]); > +} Maybe add a comment to these? Observing that... > + > +/* Fill GDB's register array with the general-purpose register values > + in *REGP. > > + When debugging a 32-bit executable running under a 64-bit kernel, > + we have to fix up the 64-bit registers we get from the kernel to > + make them look like 32-bit registers. */ > +void > +supply_gregset (struct regcache *regcache, const gregset_t *regp) > +{ ... you need a empty line between the comment of a function and the declaration of that function. > #ifdef __s390x__ > struct gdbarch *gdbarch = get_regcache_arch (regcache); > - if (offset != -1 && gdbarch_ptr_bit (gdbarch) == 32) > + if (gdbarch_ptr_bit (gdbarch) == 32) > { > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + ULONGEST pswm = 0, pswa = 0; > + gdb_byte buf[4]; > + const short *map; > > - if (regno == S390_PSWM_REGNUM) > - { > - ULONGEST pswm; > - gdb_byte buf[4]; > - > - pswm = extract_unsigned_integer (regp + regmap[S390_PSWM_REGNUM], > - 8, byte_order); > - > - store_unsigned_integer (buf, 4, byte_order, (pswm >> 32) | 0x80000); > - regcache_raw_supply (regcache, regno, buf); > - return; > - } > - > - if (regno == S390_PSWA_REGNUM) > + for (map = regmap_gregset; map[0] >= 0; map += 2) > { > - ULONGEST pswm, pswa; > - gdb_byte buf[4]; > + const gdb_byte *p = (const gdb_byte *) regp + map[0]; > + int regno = map[1]; > > - pswa = extract_unsigned_integer (regp + regmap[S390_PSWA_REGNUM], > - 8, byte_order); > - pswm = extract_unsigned_integer (regp + regmap[S390_PSWM_REGNUM], > - 8, byte_order); > - > - store_unsigned_integer (buf, 4, byte_order, > - (pswa & 0x7fffffff) | (pswm & 0x80000000)); > - regcache_raw_supply (regcache, regno, buf); > - return; > + if (regno == S390_PSWM_REGNUM) > + pswm = extract_unsigned_integer (p, 8, byte_order); > + else if (regno == S390_PSWA_REGNUM) > + pswa = extract_unsigned_integer (p, 8, byte_order); > + else > + { > + if ((regno >= S390_R0_REGNUM && regno <= S390_R15_REGNUM) > + || regno == S390_ORIG_R2_REGNUM) > + p += 4; > + regcache_raw_supply (regcache, regno, p); > + } > } > > - if ((regno >= S390_R0_REGNUM && regno <= S390_R15_REGNUM) > - || regno == S390_ORIG_R2_REGNUM) > - offset += 4; > + store_unsigned_integer (buf, 4, byte_order, (pswm >> 32) | 0x80000); > + regcache_raw_supply (regcache, S390_PSWM_REGNUM, buf); > + store_unsigned_integer (buf, 4, byte_order, > + (pswa & 0x7fffffff) | (pswm & 0x80000000)); > + regcache_raw_supply (regcache, S390_PSWA_REGNUM, buf); > + return; > } > #endif > > - if (offset != -1) > - regcache_raw_supply (regcache, regno, regp + offset); > + s390_native_supply (regcache, regmap_gregset, (const gdb_byte *) regp); > } > > -static void > -s390_native_collect (const struct regcache *regcache, int regno, > - gdb_byte *regp, int *regmap) > +/* Fill register REGNO (if it is a general-purpose register) in > + *REGP with the value in GDB's register array. If REGNO is -1, > + do this for all registers. */ > +void > +fill_gregset (const struct regcache *regcache, gregset_t *regp, int regno) Another instance of a missing empty line after the comment.