[Hurd] Add hardware watch support

  2014-08-31  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	Add hardware watch support to gnu-i386 platform.

	* gdb/gdb/gnu-nat.c (inf_threads): New function.
	* gdb/gdb/gnu-nat.h (inf_threads): New declaration.
	* gdb/gdb/i386gnu-nat.c: Include "i386-nat.h" and "inf-child.h"
        [i386_DEBUG_STATE] (DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL):
	New macros
        [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,
	i386_gnu_dr_set_control, 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
	[i386_DEBUG_STATE] (_initialize_i386gnu_nat): Initialize hardware i386
	debugging register hooks.
  

Hi Samuel,

I initially meant to reply to this to draw your attention to this
patch:

  https://sourceware.org/ml/gdb-patches/2014-09/msg00015.html

which, amongst other things, renames a bunch of files and functions
you've referenced to have "x86" prefixes rather than "i386".  But
while I was replying I started reviewing your patch.  Most of my
corrections are coding standard issues, but don't take that
personally: it happens to us all!

Samuel Thibault wrote:
> 2014-08-31  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	Add hardware watch support to gnu-i386 platform.
> 
> 	* gdb/gdb/gnu-nat.c (inf_threads): New function.
> 	* gdb/gdb/gnu-nat.h (inf_threads): New declaration.
> 	* gdb/gdb/i386gnu-nat.c: Include "i386-nat.h" and "inf-child.h"
>         [i386_DEBUG_STATE] (DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL):
> 	New macros
>         [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,
> 	i386_gnu_dr_set_control, 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
> 	[i386_DEBUG_STATE] (_initialize_i386gnu_nat): Initialize hardware i386
> 	debugging register hooks.
> 
> Index: gdb/gdb/gnu-nat.c
> ===================================================================
> --- gdb.orig/gdb/gnu-nat.c
> +++ gdb/gdb/gnu-nat.c
> @@ -987,6 +987,16 @@ inf_port_to_thread (struct inf *inf, mac
>    return 0;
>  }
>  
> +/* Interate F over threads.  */

"Iterate".

> +void
> +inf_threads (struct inf *inf, void (*f)(struct proc *thread))

Need a space between "(*f)" and "(struct proc *thread)", here and
in gnu-nat.h.  Also, because "void (*f) (struct proc *thread)" is
referenced twice it should be made a typedef, inf_threads_ftype or
some such thing.

> +{
> +  struct proc *thread;
> +
> +  for (thread = inf->threads; thread; thread = thread->next)
> +    f(thread);

Should be "f (thread)".

> +}
> +
>  
>  /* Make INF's list of threads be consistent with reality of TASK.  */
>  void
> Index: gdb/gdb/gnu-nat.h
> ===================================================================
> --- gdb.orig/gdb/gnu-nat.h
> +++ gdb/gdb/gnu-nat.h
> @@ -29,6 +29,9 @@ 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);
>  
> +/* Iterate F over threads */
> +void inf_threads (struct inf *inf, void (*f)(struct proc *thread));

As above, space between "(*f)" and "(struct proc *thread)", and the
type of F should be a typedef.

> +#ifndef DR_FIRSTADDR
> +#define DR_FIRSTADDR 0
> +#endif
> +
> +#ifndef DR_LASTADDR
> +#define DR_LASTADDR 3
> +#endif
> +
> +#ifndef DR_STATUS
> +#define DR_STATUS 6
> +#endif
> +
> +#ifndef DR_CONTROL
> +#define DR_CONTROL 7
> +#endif

These four are defined in i386-dregs.h, so this block of code is
unnecessary.

> +/* Get debug registers for thread THREAD.  */
> +
> +static void
> +i386_gnu_dr_get(struct i386_debug_state *regs, struct proc *thread)

Need space between function name and opening paren.
Here and numerous other places.

> +{
> +  mach_msg_type_number_t count = i386_DEBUG_STATE_COUNT;
> +  error_t err;
> +
> +  err = thread_get_state (thread->port, i386_DEBUG_STATE,
> + 			  (thread_state_t) regs, &count);
> +  if (err || count != i386_DEBUG_STATE_COUNT)

This should almost certainly be "err != 0".

> +    {
> +      warning (_("Couldn't fetch debug state from %s"),
> +	       proc_string (thread));
> +      return;
> +    }
> +}

The return is unnecessary, and, once removed, the braces should go.

> +/* Set debug registers for thread THREAD.  */
> +
> +static void
> +i386_gnu_dr_set(const struct i386_debug_state *regs, struct proc *thread)

Space between function name and opening paren.

> +{
> +  error_t err;
> +
> +  err = thread_set_state (thread->port, i386_DEBUG_STATE,
> +			  (thread_state_t) regs, i386_DEBUG_STATE_COUNT);
> +  if (err)
> +    {
> +      warning (_("Couldn't store debug state into %s"),
> +	       proc_string (thread));
> +      return;
> +    }
> +}

Again, "err != 0", return unnecessary, braces could go.

> +/* Set DR_CONTROL to ADDR in all threads.  */
> +
> +static void
> +i386_gnu_dr_set_control (unsigned long control)
> +{
> +  void f(struct proc *thread)
> +  {
> +    struct i386_debug_state regs;
> +    i386_gnu_dr_get(&regs, thread);

Space between function name and paren, here and twice below, and also
the same three places in i386_gnu_dr_set_addr below.

> +    regs.dr[DR_CONTROL] = control;
> +    i386_gnu_dr_set(&regs, thread);
> +  }
> +
> +  inf_update_procs (gnu_current_inf);
> +  inf_threads(gnu_current_inf, f);
> +}
> +
> +/* Set address REGNUM (zero based) to ADDR in all threads.  */
> +
> +static void
> +i386_gnu_dr_set_addr (int regnum, CORE_ADDR addr)
> +{
> +  void f(struct proc *thread)
> +  {
> +    struct i386_debug_state regs;
> +    i386_gnu_dr_get(&regs, thread);
> +    regs.dr[DR_FIRSTADDR + regnum] = addr;
> +    i386_gnu_dr_set(&regs, thread);
> +  }
> +
> +  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);

gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
Here and in i386_gnu_dr_get_addr.

The code 'regs.dr[DR_FIRSTADDR + regnum]' is basically assuming
DR_FIRSTADDR is zero, so maybe add an assertion for this at the
top of the i386_DEBUG_STATE protected part of _initialize_i386gnu_nat.

> +/* Get debug register REGNUM value from only the one LWP of PTID.  */
> +
> +static unsigned long
> +i386_gnu_dr_get_reg (ptid_t ptid, int regnum)
> +{
> +  struct i386_debug_state regs;
> +  struct proc *thread;
> +
> +  /* Make sure we know about new threads.  */
> +  inf_update_procs (gnu_current_inf);
> +
> +  thread = inf_tid_to_thread (gnu_current_inf, ptid_get_lwp (ptid));
> +  i386_gnu_dr_get(&regs, thread);

Space between func name and opening paren.

Don't worry about my patch renaming things from i386 to x86 for now.
If mine gets committed before yours it should be easy for you to fix,
just recompile and fix the errors.  If yours gets committed before
mine I'll update my patch to do the renames in this file too.

Cheers,
Gary
  

Hi Samuel,

Gary Benson wrote:
> I initially meant to reply to this to draw your attention to this
> patch:
> 
>   https://sourceware.org/ml/gdb-patches/2014-09/msg00015.html
> 
> which, amongst other things, renames a bunch of files and functions
> you've referenced to have "x86" prefixes rather than "i386".

This patch is now in, but it should be fairly straightforward for you
to fix yours up.  Just update your branch with the latest gdb/master
and fix all the build failures that come up.  The file nat/i386-dregs.h
is now nat/x86-dregs.h, and a bunch of types and functions were renamed
from i386_* to x86_*.

Cheers,
Gary

--
http://gbenson.net/