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

[Doug Evans <dje at google dot com>]

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?

Attachment: gdb-090622-gdbserver-hw-wp-4.patch.txt
Description: Text document