Re: [RFA] i386/amd64 h/w watchpoints in gdbserver

Pedro Alves
2009-06-20T23:54:25Z

On Tuesday 02 June 2009 16:36:05, Doug Evans wrote:

> RCS file: i386-low.c
> diff -N i386-low.c
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ i386-low.c  1 Jun 2009 22:02:43 -0000
> @@ -0,0 +1,660 @@

> +/* Support for 8-byte wide hw watchpoints.  */
> +#ifndef TARGET_HAS_DR_LEN_8
> +#define TARGET_HAS_DR_LEN_8 (sizeof (long) == 8)

Note: this will be trouble for Win64 (sizeof(long) == 4, sizeof (ptr) == 8)

> +/* Set in DR7 the RW and LEN fields for the I'th debug register.  */
> +#define I386_DR_SET_RW_LEN(state, i,rwlen) \
> +  do { \
> +    (state)->dr_control_mirror &= \
> +      ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
> +    (state)->dr_control_mirror |= \
> +      ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
> +  } while (0)

Spaces around operators missing, here and several other places.
I realise this is copied from GDB, but IWBN to fix this while we
go. Feel free to fix it on GDB as well, as obvious.

> +/* Whether or not to print the mirrored debug registers.  */
> +static int maint_show_dr = 0;

I see nowhere where this can be set. There are a few references to
GDB's main show-debug-regs command left behind. Could this be set with
a command line option and/or monitor command perhaps?

> +
> +/* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
> +typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
> +
> +/* Internal functions.  */
> +
> +/* Return the value of a 4-bit field for DR7 suitable for watching a
> +   region of LEN bytes for accesses of type TYPE.  LEN is assumed to
> +   have the value of 1, 2, or 4.  */
> +static unsigned i386_length_and_rw_bits (int len, enum target_hw_bp_type type);
> +
> +/* Insert a watchpoint at address ADDR, which is assumed to be aligned
> +   according to the length of the region to watch.  LEN_RW_BITS is the
> +   value of the bit-field from DR7 which describes the length and
> +   access type of the region to be watched by this watchpoint.  Return
> +   0 on success, -1 on failure.  */
> +static int i386_insert_aligned_watchpoint (struct i386_debug_reg_state *state,
> +                                          CORE_ADDR addr,
> +                                          unsigned len_rw_bits);
> +
> +/* Remove a watchpoint at address ADDR, which is assumed to be aligned
> +   according to the length of the region to watch.  LEN_RW_BITS is the
> +   value of the bits from DR7 which describes the length and access
> +   type of the region watched by this watchpoint.  Return 0 on
> +   success, -1 on failure.  */
> +static int i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
> +                                          CORE_ADDR addr,
> +                                          unsigned len_rw_bits);
> +
> +/* Insert or remove a (possibly non-aligned) watchpoint, or count the
> +   number of debug registers required to watch a region at address
> +   ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
> +   successful insertion or removal, a positive number when queried
> +   about the number of registers, or -1 on failure.  If WHAT is not a
> +   valid value, bombs through fatal.  */
> +static int i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state,
> +                                             i386_wp_op_t what,
> +                                             CORE_ADDR addr, int len,
> +                                             enum target_hw_bp_type type);

I don't think none of these forward declarations is needed?

> +\f
> +/* Implementation.  */
> +
> +/* Clear the reference counts and forget everything we knew about the
> +   debug registers.  */
> +
> +void
> +i386_low_cleanup_dregs (struct i386_debug_reg_state *state)
> +{
> +  int i;
> +
> +  ALL_DEBUG_REGISTERS(i)
> +    {
> +      state->dr_mirror[i] = 0;
> +      state->dr_ref_count[i] = 0;
> +    }
> +  state->dr_control_mirror = 0;
> +  state->dr_status_mirror  = 0;
> +}

This isn't called anywhere?

> +
> +/* Print the values of the mirrored debug registers.  This is called
> +   when maint_show_dr is non-zero.

> To set that up, type "maint
> +   show-debug-regs" at GDB's prompt.  */

Nope, this is a lie.

> +
> +static void
> +i386_show_dr (struct i386_debug_reg_state *state,
> +             const char *func, CORE_ADDR addr,
> +             int len, enum target_hw_bp_type type)
> +{
> +  int i;
> +
> +  printf (func);
> +  if (addr || len)
> +    printf (" (addr=%lx, len=%d, type=%s)",
> +                      (unsigned long) addr, len,
> +                      type == hw_write ? "data-write"
> +                      : (type == hw_read ? "data-read"
> +                         : (type == hw_access ? "data-read/write"
> +                            : (type == hw_execute ? "instruction-execute"
> +                               /* FIXME: if/when I/O read/write
> +                                  watchpoints are supported, add them
> +                                  here.  */
> +                               : "??unknown??"))));
> +  printf (":\n");
> +  printf ("\tCONTROL (DR7): %08x          STATUS (DR6): %08x\n",
> +                    state->dr_control_mirror, state->dr_status_mirror);
> +  ALL_DEBUG_REGISTERS (i)
> +    {
> +      printf ("\
> +\tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
> +                        i, paddr (state->dr_mirror[i]), state->dr_ref_count[i],
> +                        i+1, paddr (state->dr_mirror[i+1]), state->dr_ref_count[i+1]);
> +      i++;
> +    }
> +}

These should not go to stdout. GDB should be fixed too to make these go to
gdb_stdlog.

> +
> +static int
> +Z_packet_to_hw_type (char type)
> +{
> +  switch (type)
> +    {
> +    case Z_PACKET_WRITE_WP:  return hw_write;
> +    case Z_PACKET_READ_WP:   return hw_read;
> +    case Z_PACKET_ACCESS_WP: return hw_access;

Please don't vertically align. Stick to the standards and put
those return statements in their own line.

> +    default:
> +      error ("Z_packet_to_hw_type: bad watchpoint type %c", type);
> +    }
> +}
> +
> +/* Insert a watchpoint to watch a memory region which starts at
> +   address ADDR and whose length is LEN bytes.  Watch memory accesses
> +   of the type TYPE_FROM_PACKET.  Return 0 on success, -1 on failure.  */
> +
> +int
> +i386_low_insert_watchpoint (struct i386_debug_reg_state *state,
> +                           char type_from_packet, CORE_ADDR addr, int len)
> +{
> +  int retval;
> +  int type = Z_packet_to_hw_type (type_from_packet);
> +
> +  if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))

More examples of missing space around operators coming from GDB.

> +
> +/* If the inferior has some watchpoint that triggered, set the
> +   address associated with that watchpoint and return non-zero.
> +   Otherwise, return zero.  */
> +
> +CORE_ADDR
> +i386_low_stopped_data_address (struct i386_debug_reg_state *state)
> +{
> +  CORE_ADDR addr = 0;
> +  int i;
> +
> +  /* Get dr_status_mirror for use by I386_DR_WATCH_HIT.  */
> +  i386_dr_low_get_status (state);
> +
> +  ALL_DEBUG_REGISTERS(i)
> +    {
> +      if (I386_DR_WATCH_HIT (state, i)
> +         /* This second condition makes sure DRi is set up for a data
> +            watchpoint, not a hardware breakpoint.  The reason is
> +            that GDB doesn't call the target_stopped_data_address
> +            method except for data watchpoints.  In other words, I'm
> +            being paranoiac.  */
> +         && I386_DR_GET_RW_LEN (state, i) != 0)
> +       {
> +         addr = state->dr_mirror[i];
> +         if (maint_show_dr)
> +           i386_show_dr (state, "watchpoint_hit", addr, -1, hw_write);
> +       }
> +    }
> +
> +  if (maint_show_dr && addr == 0)
> +    i386_show_dr (state, "stopped_data_addr", 0, 0, hw_write);
> +

> +  /* NOTE: gdb version checks rc != 0 here.  */
> +  return addr;
> +}

Indeed. GDB's interface was fixed to allow watchpoints at 0. I
don't think that matters for any gdbserver target, but it would
be nice to fix the interface anyway...

> +
> +int
> +i386_low_stopped_by_watchpoint (struct i386_debug_reg_state *state)
> +{
> +  CORE_ADDR addr = 0;
> +  /* NOTE: gdb version passes boolean found/not-found result from
> +     i386_stopped_data_address.  */
> +  addr = i386_low_stopped_data_address (state);
> +  return (addr != 0);
> +}

Same as above. You've probably thought about that too...

> +\f
> +/* Support for h/w breakpoints.
> +   This support is not currently used, kept for reference.  */

Any reason for not using this currently? If there's a good reason,
than let's drop it. But I'd prefer to have it working. :-)

> Index: utils.c
> ===================================================================

+char *
+paddr (CORE_ADDR addr)

This isn't documented in neither server.h or here?

+{
+ char *str = get_cell ();
+ xsnprintf (str, CELLSIZE, "%lx", (long) addr);

^^^^

Note: this will be wrong on Win64... BTW, Ulrich
was removing several of these functions from GDB
in the removing-current_gdbarch series. Will this one
stay? Might be worth it to use the one that is going
to stay in GDB.

+ return str;
+}

> Index: linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.105
> diff -u -p -r1.105 linux-low.c
> --- linux-low.c 24 May 2009 17:44:19 -0000      1.105
> +++ linux-low.c 1 Jun 2009 22:02:43 -0000
> @@ -224,6 +224,7 @@ delete_lwp (struct lwp_info *lwp)
>  {
>    remove_thread (get_lwp_thread (lwp));
>    remove_inferior (&all_lwps, &lwp->head);
> +  free (lwp->arch_private);
>    free (lwp);
>  }
>
> @@ -242,6 +243,9 @@ linux_add_process (int pid, int attached
>    proc = add_process (pid, attached);
>    proc->private = xcalloc (1, sizeof (*proc->private));
>
> +  if (the_low_target.new_process != NULL)
> +    proc->private->arch_private = the_low_target.new_process (pid, attached);

( Wouldn't the interface be a bit cleaner if you
passed the 'proc' pointer down to new_process, and have the
callback manage arch_private field itself? As is, your passing
some useless parameters down. If they end up being needed,
it is likely that an extra look up on pid would be needed to
get at the structure. This would also migrate better to a
per-process data mechanism similar to gdb's objfile_data, if
need be. )

> Index: linux-low.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 linux-low.h
> --- linux-low.h 12 May 2009 22:25:00 -0000      1.30
> +++ linux-low.h 1 Jun 2009 22:02:43 -0000
> @@ -56,8 +56,13 @@ struct process_info_private
>
>    /* Connection to the libthread_db library.  */
>    td_thragent_t *thread_agent;
> +
> +  /* Target-specific additions.  */

Warning: "Target" overload. We need to get into the habit
of not doing this --- it makes refering to these things quite
ambiguous. Call it "arch" or something else. There are other
similar cases.

>  #define pid_of(proc) ptid_get_pid ((proc)->head.id)
>  #define lwpid_of(proc) ptid_get_lwp ((proc)->head.id)
> +#define PIDGET(ptid) ptid_get_pid (ptid)
> +#define TIDGET(ptid) ptid_get_lwp (ptid)

Why do we need extra ways to do the same thing? Let's not
copy GDB's bad habits and legacy code.

> --- linux-x86-low.c     13 May 2009 19:11:04 -0000      1.2
> +++ linux-x86-low.c     1 Jun 2009 22:02:43 -0000

> +static unsigned long
> +x86_linux_dr_get (ptid_t ptid, int regnum)
> +{
> +  int tid;
> +  unsigned long value;
> +
> +  tid = TIDGET (ptid);
> +  if (tid == 0)
> +    tid = PIDGET (ptid);

The tid == 0 case is dead code coming from GDB, isn't it?
Likewise in other places.

> +
> +  errno = 0;
> +  value = ptrace (PTRACE_PEEKUSER, tid,
> +                 offsetof (struct user, u_debugreg[regnum]), 0);
> +  if (errno != 0)
> +    error ("Couldn't read debug register");
> +
> +  return value;
> +}
> +

> +/* Update the inferior's debug register REGNUM from STATE.  */
> +
> +void
> +i386_dr_low_set_addr (const struct i386_debug_reg_state *state, int regnum)
> +{
> +  struct inferior_list_entry *lp;
> +  CORE_ADDR addr;
> +
> +  if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
> +    fatal ("Invalid debug register %d", regnum);
> +
> +  addr = state->dr_mirror[regnum];
> +
> +  /* ??? Will need tweaking for multi-process.  */

Indeed. Why not just set the debug_registers_changed in lwps
of the current process?

> +  for (lp = all_lwps.head; lp; lp = lp->next)
> +    {
> +      struct lwp_info *lwp = (struct lwp_info *) lp;
> +
> +      /* The actual update is done later, we just mark that the register
> +        needs updating.  */
> +      lwp->arch_private->debug_registers_changed = 1;
> +    }

Thanks, this is likely more non-stop friendly than the win32 version.
We can just send a SIGSTOP to each thread that is stopped, and set
stop_expected. Then, the debug register updating just works. (we'll
still need to make sure that moribund locations work with watchpoints
though)

> Index: utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/utils.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 utils.c
> --- utils.c     19 Jan 2009 00:16:46 -0000      1.18
> +++ utils.c     1 Jun 2009 22:02:43 -0000
> @@ -170,3 +170,37 @@ warning (const char *string,...)
>    fprintf (stderr, "\n");
>    va_end (args);
>  }
> +
> +/* temporary storage using circular buffer */

More bogus formatting copied from GDB. Full sentence: capitalize, period,
double space.

> +#define NUMCELLS 4
> +#define CELLSIZE 50
> +static char *
> +get_cell (void)
> +{
> +  static char buf[NUMCELLS][CELLSIZE];
> +  static int cell = 0;
> +  if (++cell >= NUMCELLS)
> +    cell = 0;
> +  return buf[cell];
> +}
> +

Otherwise, this is looking good to me. I still feel that
exposing the debug registers to GDB (as per-process wide
registers), as opposed to using Z packets would be an
alternative worth investigating, that would avoid having
to mostly copy i386-nat.c to gdbserver/i386-low.c. But,
the Z packets interface already exists, so, that can
always be investigated at any later time...

--
Pedro Alves