From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31797 invoked by alias); 12 Sep 2014 16:51:55 -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 31788 invoked by uid 89); 12 Sep 2014 16:51:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 12 Sep 2014 16:51:52 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E7455116457; Fri, 12 Sep 2014 12:51:50 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3sEdNPixeEhT; Fri, 12 Sep 2014 12:51:50 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 917F3116453; Fri, 12 Sep 2014 12:51:50 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 8501B40E17; Fri, 12 Sep 2014 09:51:49 -0700 (PDT) Date: Fri, 12 Sep 2014 16:51:00 -0000 From: Joel Brobecker To: Gary Benson , bug-hurd@gnu.org, thomas@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCHv3,Hurd] Add hardware watch support Message-ID: <20140912165149.GD4871@adacore.com> References: <20140910224919.GP3244@type.youpi.perso.aquilenet.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140910224919.GP3244@type.youpi.perso.aquilenet.fr> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-09/txt/msg00437.txt.bz2 > 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. In addition to Sergio's comments, I noticed: You forgot the comment I made about having the function documented at only one location, and the contents of that documentat. For easy reference, here are my comments again: | Use... | | /* See gnu-nat.h. */ | | ... instead. This is a fairly trivial comment in this case, so | not biggie, but the idea is that want to avoid duplicating | documentation in order to avoid having one of them being out | of sync. | | And please also make sure to always have an empty line before | function documentation and start of implementation. | | > void | > diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h | > index 8e949eb..011c38c 100644 | > --- a/gdb/gnu-nat.h | > +++ b/gdb/gnu-nat.h | > @@ -29,6 +29,11 @@ extern struct inf *gnu_current_inf; | > /* Converts a GDB pid to a struct proc. */ | > struct proc *inf_tid_to_thread (struct inf *inf, int tid); | > | > +typedef void (inf_threads_ftype) (struct proc *thread); | > + | > +/* Iterate F over threads. */ | | Suggest: | | /* Call F for every thread in inferior INF. */ | | This addresses two issues: "Iterate F" sounds odd, but perhaps | that's because English is not my native language; but also, | it also documents what INF is. > +static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg) > +{ For the function implementations, the name of the function should be at the first column on the next line. This is a GNU Coding Style (GCS) requirement, and allows easy searching of a function's body by grepping for "^FUNCTION_NAME". I also think it helps having the name of the function for each hunk in "diff -p" (not completely sure about that one). > + unsigned long *control = arg; > + struct i386_debug_state regs; > + i386_gnu_dr_get (®s, thread); I just wanted to add that Sergio's request to add an empty line after the variable declarations is actually a rule in the GDB project. For the record, our coding standards are documented at: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards It's not complete yet, so do not be too surprise if we come up with things that you haven't seen there or in the GCS. Do call us on it, though, and we will update the doc. > +struct reg_addr { > + int regnum; > + CORE_ADDR addr; > +}; While touching this code, we tend to prefer it for struct definitions if the opening curly brace is at the start of the next line, please. struct reg_addr { int regnum; CORE_ADDR addr; }; > +static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg) Same as above. > + struct reg_addr reg_addr; > + gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR); Missing empty line between var declaration and rest of code. -- Joel