Re: [patch 1/4] hw watchpoints code cleanups

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Mon, 6 Dec 2010 12:12:07 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Hi,
> 
> this is just some mechanical moving of code around to make the real code
> changes later better visible.
> 
>  static void
>  amd64_linux_dr_set_control (unsigned long control)
>  {
> -  struct lwp_info *lp;
> -  ptid_t ptid;
> -
>    amd64_linux_dr[DR_CONTROL] = control;
> -  ALL_LWPS (lp, ptid)
> -    amd64_linux_dr_set (ptid, DR_CONTROL, control);
> +
> +  iterate_over_lwps (minus_one_ptid, amd64_linux_dr_set_control_callback,
> +		     (void *) control);

It really is bad style to assume you can cast an integer to (void *),
even if you know the pointer has the same width as the integer.  If
you really want to go that route, pass &control, and adjust
amd64_linux_dr_set_control_callback accordingly.  There are morecases
like this in this diff.  Please fix them all.

> --- a/gdb/i386-nat.c
> +++ b/gdb/i386-nat.c
> @@ -25,6 +25,7 @@
>  #include "gdbcmd.h"
>  #include "target.h"
>  #include "gdb_assert.h"
> +#include "inferior.h"
>  
>  /* Support for hardware watchpoints and breakpoints using the i386
>     debug registers.
> @@ -104,6 +105,19 @@ struct i386_dr_low_type i386_dr_low;
>     FIXME: My Intel manual says we should use 0xF800, not 0xFC00.  */
>  #define DR_CONTROL_RESERVED	(0xFC00)
>  
> +/* Copy of hardware debug registers for performance reasons.  */
> +
> +struct dr_mirror

struct i386_dr_mirror please.

> +  {
> +    /* Mirror the inferior's DRi registers.  We keep the status and
> +       control registers separated because they don't hold addresses.  */
> +    CORE_ADDR addr[DR_NADDR];
> +    unsigned long status, control;
> +
> +    /* Reference counts for each debug register.  */
> +    int ref_count[DR_NADDR];
> +  };
> +
>  /* Auxiliary helper macros.  */
>  
>  /* A value that masks all fields in DR7 that are reserved by Intel.  */
> @@ -111,49 +125,60 @@ struct i386_dr_low_type i386_dr_low;
>  
>  /* The I'th debug register is vacant if its Local and Global Enable
>     bits are reset in the Debug Control register.  */
> -#define I386_DR_VACANT(i) \
> -  ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
> +
> +static inline int
> +dr_vacant (struct dr_mirror *dr_mirror, int i)
> +{
> +  return (dr_mirror->control & (3 << (DR_ENABLE_SIZE * i))) == 0;
> +}

Please keep the i386_ prefix in all these functions.