From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5239 invoked by alias); 10 Feb 2016 13:34:59 -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 5222 invoked by uid 89); 10 Feb 2016 13:34:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Great, HContent-Transfer-Encoding:8bit X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 Feb 2016 13:34:57 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id CE3053FEBC; Wed, 10 Feb 2016 14:35:44 +0100 (CET) Received: from [192.168.1.62] (84-10-2-59.static.chello.pl [84.10.2.59]) by hogfather.0x04.net (Postfix) with ESMTPSA id 72CDB58008C; Wed, 10 Feb 2016 14:34:54 +0100 (CET) Subject: Re: [PATCH 5/6] gdb/x86: Implement ax_pseudo_register_collect hook. To: Pedro Alves , gdb-patches@sourceware.org References: <1454773157-31569-1-git-send-email-koriakin@0x04.net> <1454792070-25410-1-git-send-email-koriakin@0x04.net> <56BB3BAA.3050908@redhat.com> From: =?UTF-8?Q?Marcin_Ko=c5=9bcielnicki?= Message-ID: <56BB3C7E.4080102@0x04.net> Date: Wed, 10 Feb 2016 13:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56BB3BAA.3050908@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00284.txt.bz2 On 10/02/16 14:31, Pedro Alves wrote: > Great stuff. Minor comments follow... > > Patch is missing intro comments to new functions: > > /* Implementation of foo. / See foo.h. / etc. */ OK. > > On 02/06/2016 08:54 PM, Marcin Kościelnicki wrote: > >> +static int >> +amd64_ax_pseudo_register_collect (struct gdbarch *gdbarch, >> + struct agent_expr *ax, int regnum) >> +{ >> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > Empty line after decl here. OK. > > >> +int >> +i386_ax_pseudo_register_collect (struct gdbarch *gdbarch, >> + struct agent_expr *ax, int regnum) >> +{ >> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > Ditto. > >> + if (i386_mmx_regnum_p (gdbarch, regnum)) >> + { >> + /* MMX to FPU register mapping depends on current TOS. Let's just >> + not care and collect everything... */ >> + int i; > > Ditto. (several instances more in the function) > >> + ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep)); >> + for (i = 0; i < 8; i++) >> + ax_reg_mask (ax, I387_ST0_REGNUM (tdep) + i); >> + return 0; >> + } > > >> + else if (i386_byte_regnum_p (gdbarch, regnum)) >> + { >> + /* Check byte pseudo registers last since this function will >> + be called from amd64_ax_pseudo_register_collect, which handles >> + byte pseudo registers differently. */ > > I don't understand this comment. amd64_ax_pseudo_register_collect > checks i386_byte_regnum_p before ever reaching here, afaics. I've copied it from i386_pseudo_register_read_into_value - didn't make that much sense to me either. Let's remove it from both places? > >> + int gpnum = regnum - tdep->al_regnum; >> + ax_reg_mask (ax, gpnum % 4); >> + return 0; >> + } >> + else >> + internal_error (__FILE__, __LINE__, _("invalid regnum")); >> + return 1; >> +} > > Thanks, > Pedro Alves >