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

[Jan Kratochvil <jan.kratochvil@redhat.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