Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.

Jan Kratochvil
2014-06-22T18:31:30Z

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