On Sat, Jun 20, 2009 at 4:55 PM, Pedro Alves<pedro@codesourcery.com> wrote:
> 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)

changed to sizeof (void *)

>> +/* 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.

patch updated.

>> +/* 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?

It was useful during debugging, I just hardcoded it to 1.
As for how to really enable set it, I wasn't sure.
I added a monitor command.

>
>> +
>> +/* 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?

ok
[once upon a time, they were ok. the new rules haven't been locked in
memory yet]

>> +
>> +/* 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?

Ah.  'tis now.

>> +
>> +/* 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.

fixed.

>> +
>> +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.

changed to stderr

>> +
>> +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.

k.

>> +    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.

patch updated

>> +
>> +/* 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...

patch updated

>> +
>> +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...
>
>> +
>> +/* 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.  :-)

deleted.

>> Index: utils.c
>> ===================================================================
>
> +char *
> +paddr (CORE_ADDR addr)
>
> This isn't documented in neither server.h or here?

Just "going with the flow".

> +{
> +  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.

I think a higher order bit is that gdb and gdbserver cannot share
code.  Bringing over all the smarts to handle all the different
portability issues is painful/depressing.  I went with something
simple that works for now.
IWBN if this kind of thing were in, say, libiberty and tools could just use it.

>
> +  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. )

args deleted.

>> 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.

I dunno.   there's "the_low_target" in linux-low.h
Perhaps we can migrate away but I don't see the above "infraction" as
being critical.

>>  #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.

k.

>> --- 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.

Perhaps.  There's similar code in linux-low.c:same_lwp.
== 0 code deleted.

>> +
>> +  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?

Are there any existing examples of this?
I would have done that had process_info contained the list of its
threads (it would have been trivially straightforward).

>> +  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.

k.

>> +#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...

Setting aside breakpoints+watchpoints -> "points",
http://sourceware.org/ml/gdb-patches/2009-06/msg00594.html
how about this?