[patch] aarch64: PR 19806: watchpoints: false negatives -> false positives

[Pedro Alves]

On 06/19/2016 07:29 PM, Jan Kratochvil wrote:
> On Wed, 08 Jun 2016 20:46:35 +0200, Pedro Alves wrote:
>> I thought of a case where this is the wrong thing to do.
>> (Alternative below.)
>>
>> The same example as before: e.g., a machine that only supports
>> watching 32-bit-aligned words.  Then with:
>>
>> union
>> {
>>   char buf[4];
>>   uint32_t force_align;
>> } global;
>>
>> (gdb) watch global.buf[1];
>> Hardware watchpoint 1 ...
>> (gdb) watch global.buf[3];
>> Hardware watchpoint 2 ...
>>
>> ... if the program writes to global.buf[3], and the target
>> reports a memory access to 'global.buf + 1' (because that's the
>> first watchpoint in its own watchpoint list that overlaps
>> the global.buf[0]..global.buf[3] range (what is really being
>> watched)), gdb will believe that watchpoint 1 triggered, notice
>> the value didn't change, and thus incorrectly ignore the watchpoint hit.
> 
> Here if the program really does 'global.buf[3] = 0xff;' then it still does
> work as despite GDB places watchpoint at &global.buf[0] for 4 bytes aarch64
> still reports the exact 1-byte location &global.buf[3]:
> 	echo -e 'union { char buf[8]; unsigned long ul; } u; int main(void) {\n u.buf[3]=0xff;\n return 0; }'|gcc -Wall -g -x c -;./gdb -data-directory ./data-directory/ ./a.out -ex start -ex 'watch u.buf[1]' -ex 'watch u.buf[3]' -ex c -ex 'p u.buf[1]' -ex 'p u.buf[3]'
> 	Hardware watchpoint 2: u.buf[1]
> 	Hardware watchpoint 3: u.buf[3]
> 	Continuing.
> 	Hardware watchpoint 3: u.buf[3]
> 	Old value = 0 '\000'
> 	New value = 255 '\377'

Sounds what would happen when gdbserver looks for the first watchpoint
that overlaps the kernel-reported siginfo.si_addr trap address, it finds
watchpoint 3 first.  Which is entirely plausible because gdbserver actually
iterates backwards:

  /* Check if the address matches any watched address.  */
  state = aarch64_get_debug_reg_state (pid_of (current_thread));
  for (i = aarch64_num_wp_regs - 1; i >= 0; --i)
    {
      const unsigned int len = aarch64_watchpoint_length (state->dr_ctrl_wp[i]);

So I guess that if you manage to reverse the order of the watchpoints
in gdbserver, gdbserver will find u.buf[1] as first overlapping watchpoint,
and then we'd see what I described.  Or if you reverse the iteration
order in that for loop.

Maybe just setting the watchpoint at buf[3] first would be sufficient.

 (gdb) watch global.buf[3];
 Hardware watchpoint 1 ...
 (gdb) watch global.buf[1];
 Hardware watchpoint 2 ...
 ...

Thanks,
Pedro Alves