From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2939 invoked by alias); 10 Sep 2014 23:23:06 -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 2929 invoked by uid 89); 10 Sep 2014 23:23:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 10 Sep 2014 23:23:03 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8ANMwd6002053 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 10 Sep 2014 19:22:58 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8ANMuAm021944 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 10 Sep 2014 19:22:57 -0400 From: Sergio Durigan Junior To: Joel Brobecker Cc: Gary Benson , bug-hurd@gnu.org, thomas@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCHv3,Hurd] Add hardware watch support References: <20140910224919.GP3244@type.youpi.perso.aquilenet.fr> X-URL: http://www.redhat.com Date: Wed, 10 Sep 2014 23:23:00 -0000 In-Reply-To: <20140910224919.GP3244@type.youpi.perso.aquilenet.fr> (Samuel Thibault's message of "Thu, 11 Sep 2014 00:49:19 +0200") Message-ID: <87egvjawmn.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00348.txt.bz2 On Wednesday, September 10 2014, Samuel Thibault wrote: > 2014-09-06 Samuel Thibault > > Add hardware watch support to gnu-i386 platform. > > * gdb/gdb/gnu-nat.c (inf_threads): New function. > * gdb/gdb/gnu-nat.h (inf_threads_ftype): New type. > (inf_threads): New declaration. > * gdb/gdb/i386gnu-nat.c: Include "x86-nat.h" and "inf-child.h". > [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set, > i386_gnu_dr_set_control_one, i386_gnu_dr_set_control, > i386_gnu_dr_set_addr_one, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg, > i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control): > New functions > (reg_addr): New structure. > (_initialize_i386gnu_nat) [i386_DEBUG_STATE]: Initialize hardware i386 > debugging register hooks. Thanks for the patch, Samuel. What do you think of writing a message explaining a bit more of this feature, for the sake of putting it in the commit message? Just thinking aloud here... I only have comments about style and organization of the code. > diff --git a/gdb/i386gnu-nat.c b/gdb/i386gnu-nat.c > index 8fad871..5654e9a 100644 > --- a/gdb/i386gnu-nat.c > +++ b/gdb/i386gnu-nat.c [...] > +static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg) > +{ > + unsigned long *control = arg; > + struct i386_debug_state regs; > + i386_gnu_dr_get (®s, thread); > + regs.dr[DR_CONTROL] = *control; > + i386_gnu_dr_set (®s, thread); > +} This function needs a comment, and the organization is a little messy. Could you put a newline after the declaration of the variables? [...] > +struct reg_addr { > + int regnum; > + CORE_ADDR addr; > +}; This structure also needs a comment, and we put a comment on each struct field as well. > +static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg) > +{ > + struct reg_addr *reg_addr = arg; > + struct i386_debug_state regs; > + i386_gnu_dr_get (®s, thread); > + regs.dr[reg_addr->regnum] = reg_addr->addr; > + i386_gnu_dr_set (®s, thread); > +} Same comment from the function above: missing a comment, and newline after var declaration. Is it possible to submit a testcase for this as well? WDYT? Thanks, -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/