This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Doug Evans <xdje42 at gmail dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Sun, 22 Jun 2014 20:31:10 +0200
- Subject: Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
- Authentication-results: sourceware.org; auth=none
- References: <1394154640-14053-3-git-send-email-palves at redhat dot com> <CAP9bCMTj+A2B6cvwYKC2vjL1Si+yGBKXvQfCMNR0SYN19KztXg at mail dot gmail dot com> <53272CB0 dot 6050101 at redhat dot com> <532AF3D0 dot 8090904 at redhat dot com> <20140617191850 dot GA10997 at host2 dot jankratochvil dot net> <20140619134330 dot GA14567 at host2 dot jankratochvil dot net> <53A2FB68 dot 9090500 at redhat dot com> <53A3164E dot 4020109 at redhat dot com> <20140619170029 dot GA31275 at host2 dot jankratochvil dot net> <53A46706 dot 8010601 at redhat dot com>
On Fri, 20 Jun 2014 18:53:26 +0200, Pedro Alves wrote:
> I could reproduce this with 7.7, even.
>
> This is not a 7.8 regression, but I think it can go in safely to the branch.
While I agree it still is a testsuite results regression.
[...]
> --- a/gdb/nat/i386-dregs.c
> +++ b/gdb/nat/i386-dregs.c
> @@ -141,6 +141,13 @@
> (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
> } while (0)
>
> +/* Locally disable the break/watchpoint in the I'th debug register. */
> +#define I386_DR_LOCAL_DISABLE(state, i) \
> + do { \
> + (state)->dr_control_mirror &= \
> + ~(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
> + } while (0)
> +
> /* Globally enable the break/watchpoint in the I'th debug register. */
> #define I386_DR_GLOBAL_ENABLE(state, i) \
> do { \
> @@ -348,6 +355,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
> CORE_ADDR addr, unsigned len_rw_bits)
> {
> int i, retval = -1;
> + int all_vacant = 1;
>
> ALL_DEBUG_REGISTERS (i)
> {
> @@ -360,11 +368,31 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
> /* Reset our mirror. */
> state->dr_mirror[i] = 0;
> I386_DR_DISABLE (state, i);
> + /* Even though not strictly necessary, clear out all
> + bits in DR_CONTROL related to this debug register.
> + Debug output is clearer when we don't have stale bits
> + in place. This also allows the assertion below. */
> + I386_DR_LOCAL_DISABLE (state, i);
This is redundant, I386_DR_DISABLE already does both GLOBAL_DISABLE-like and
LOCAL_DISABLE-like.
> + I386_DR_SET_RW_LEN (state, i, 0);
> }
> retval = 0;
> }
> +
> + if (!I386_DR_VACANT (state, i))
> + all_vacant = 0;
> }
>
> + if (all_vacant)
> + {
> + /* Even though not strictly necessary, clear out all of
> + DR_CONTROL, so that when we have no debug registers in use,
> + we end up with DR_CONTROL == 0. The Linux support relies on
> + this for an optimization. Plus, it makes for clearer debug
> + output. */
> + state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
> +
> + gdb_assert (state->dr_control_mirror == 0);
> + }
> return retval;
> }
>
[...]
Thanks,
Jan