Re: [PATCH 2/3] Move mips hardware watchpoint stuff to common/

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Thu, 13 Jun 2013, Yao Qi wrote:

> 2013-06-13  Yao Qi  <yao@codesourcery.com>

 Thanks for this work.  Please see the individual notes below.

> 	* Makefile.in (HFILES_NO_SRCDIR): Add common/mips-linux-watch.h.
> 	(mips-linux-watch.o): New rule.
> 	* common/mips-linux-watch.c, common/mips-linux-watch.h: New.

 Please list these files individually.

> 	* config/mips/linux.mh (NAT_FILE): Add mips-linux-watch.o.

 The `make' variable adjusted is called NATDEPFILES, not NAT_FILE.

> 	* mips-linux-nat.c: Include mips-linux-watch.h.
> 	(W_BIT, R_BIT, I_BIT, W_MASK, R_MASK, I_MASK, IRW_MASK): Move
> 	to common/mips-linux-watch.h.
> 	(MAX_DEBUG_REGISTER): Likewise.
> 	(struct mips_watchpoint): Likewise.
> 	(get_irw_mask, get_reg_mask, get_num_valid, get_watchlo): Move to
> 	to common/mips-linux-watch.c and add name prefix 'mips_linux_watch_'.

 Repeated "to", see below however.

> diff --git a/gdb/common/mips-linux-watch.c b/gdb/common/mips-linux-watch.c
> new file mode 100644
> index 0000000..eeac2f8
> --- /dev/null
> +++ b/gdb/common/mips-linux-watch.c
> @@ -0,0 +1,359 @@
> +/* Copyright (C) 2009-2013 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <sys/ptrace.h>
> +#include "mips-linux-watch.h"
> +#include "gdb_assert.h"
> +
> +#ifdef GDBSERVER
> +#define hw_write '2'
> +#define hw_read '3'
> +#define hw_access '4'
> +#else
> +#include "breakpoint.h"
> +#endif
> +
> +/* Assuming usable watch registers REGS, return the num_valid.  */
> +
> +uint32_t
> +mips_linux_watch_get_num_valid (struct pt_watch_regs *regs)
> +{
> +  switch (regs->style)
> +    {
> +    case pt_watch_style_mips32:
> +      return regs->mips32.num_valid;
> +    case pt_watch_style_mips64:
> +      return regs->mips64.num_valid;
> +    default:
> +      internal_error (__FILE__, __LINE__,
> +		      _("Unrecognized watch register style"));
> +    }
> +}
> +
> +/* Assuming usable watch registers REGS, return the irw_mask of
> +   register N.  */
> +
> +uint32_t
> +mips_linux_watch_get_irw_mask (struct pt_watch_regs *regs, int n)

 Any particular reason with the move of this function from 
mips-linux-nat.c to mips-linux-watch.c you renamed the SET argument to N 
(while keeping the old name in the prototype in mips-linux-watch.h)?

 You're moving pieces of code between files while at the same time making 
subtle changes.  This makes it easy for something to escape unnoticed.  
In some cases it may be unavoidable, here however it does not appear 
to me you need to make both sets of changes at once.

 Please therefore split this change into two self-contained stages:

1. All the tweaks including function renames, their API changes, comment 
   updates, etc., applied while still a part of mips-linux-nat.c.

2. The move of the adjusted pieces verbatim over to mips-linux-watch.[ch].

Of course you'll still have to make some trivial changes in the course of 
the move, in particular adding/removing header inclusions as necessary and 
adding the conditional stuff around #ifdef GDBSERVER.

 Let me know if you think you may have any trouble with that.

 Barring the pieces that are moved from mips-linux-nat.c to 
mips-linux-watch.[ch] this patch looks good, thanks.  To hopefully save us 
from extra iteration here are some notes on issues in mips-linux-watch.h 
that are readily visible and that you can address in the stage #2 referred 
above.

 Also in the stage #1 (or better yet, as a preparatory #0 patch) please 
make the type of irw variables consistent; "unsigned" is a K&R-ism, 
"uint32_t" will be better as it's already used in many places for this 
purpose ("uint16_t" is used in the kernel structures, but let's leave the 
consideration of any change to this type for another day).

> diff --git a/gdb/common/mips-linux-watch.h b/gdb/common/mips-linux-watch.h
> new file mode 100644
> index 0000000..5d4fa00
> --- /dev/null
> +++ b/gdb/common/mips-linux-watch.h
> @@ -0,0 +1,76 @@
> +/* Copyright (C) 2009-2013 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef MIPS_LINUX_WATCH_H
> +#define MIPS_LINUX_WATCH_H 1
> +
> +#ifdef GDBSERVER
> +#include "server.h"
> +#else
> +#include "defs.h"
> +#endif
> +
> +#include <asm/ptrace.h>
> +#include <stdint.h>
> +
> +#define W_BIT 0
> +#define R_BIT 1
> +#define I_BIT 2
> +
> +#define W_MASK (1 << W_BIT)
> +#define R_MASK (1 << R_BIT)
> +#define I_MASK (1 << I_BIT)
> +
> +#define IRW_MASK (I_MASK | R_MASK | W_MASK)
> +
> +#define MAX_DEBUG_REGISTER 8
> +
> +/* We keep list of all watchpoints we should install and calculate the
> +   watch register values each time the list changes.  This allows for
> +   easy sharing of watch registers for more than one watchpoint.  */
> +
> +struct mips_watchpoint
> +{
> +  CORE_ADDR addr;
> +  int len;
> +  int type;
> +  struct mips_watchpoint *next;
> +};
> +
> +uint32_t mips_linux_watch_get_num_valid (struct pt_watch_regs *regs);
> +uint32_t mips_linux_watch_get_irw_mask (struct pt_watch_regs *regs,
> +					int set);
> +CORE_ADDR mips_linux_watch_get_watchlo (struct pt_watch_regs *regs,
> +					int set);

 The two prototypes above will fit in a single line each.

> +void mips_linux_watch_set_watchlo (struct pt_watch_regs *regs,
> +				   int set, CORE_ADDR value);
> +uint32_t mips_linux_watch_get_watchhi (struct pt_watch_regs *regs,
> +				       int n);

 As will one above.

> +void mips_linux_watch_set_watchhi (struct pt_watch_regs *regs, int n,
> +				   uint16_t value);

 Please carry "int n" onto the second line for consistency with 
mips_linux_watch_set_watchlo.

> +int mips_linux_watch_try_one_watch (struct pt_watch_regs *regs,
> +				    CORE_ADDR addr, int len,
> +				    unsigned irw);

 The three last arguments above will fit in a single line.

 Please resubmit with the changes requested.

  Maciej