Bug 19806 - Aarch64: watchpoints set on non-8-byte-aligned addresses are always missed
Summary: Aarch64: watchpoints set on non-8-byte-aligned addresses are always missed
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: breakpoints (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 8.2
Assignee: Jan Kratochvil
URL:
Keywords:
Depends on:
Blocks: 20207 22389
  Show dependency treegraph
 
Reported: 2016-03-10 19:56 UTC by Pedro Alves
Modified: 2018-05-04 20:31 UTC (History)
1 user (show)

See Also:
Host:
Target: aarch64-*
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Alves 2016-03-10 19:56:50 UTC
For example, with:

union
{
  char buf[4];
  unsigned int ul;
} u;

int
main ()
{
  u.ul = 0xffffffff;
  return 0;
}

on x86-64, we get:

(gdb) watch u.buf[1]
Hardware watchpoint 1: u.buf[1]
(gdb) c
Continuing.
Hardware watchpoint 1: u.buf[1]

Old value = 0 '\000'
New value = -1 '\377'
main () at watch.c:11
11              return 0;
(gdb) 

While on Aarch64, gdb just miss the watchpoint hit.

Actually, the kernel reports the hit to gdb, and linux-nat.c forwards the event to infrun.c.

However, it doesn't work as expected because this:


int
watchpoints_triggered (struct target_waitstatus *ws)
{
...
  ALL_BREAKPOINTS (b)
    if (is_hardware_watchpoint (b))
      {
...
	for (loc = b->loc; loc; loc = loc->next)
	  {
...
	    /* Exact match not required.  Within range is sufficient.  */
	    else if (target_watchpoint_addr_within_range (&current_target,
							 addr, loc->address,
							 loc->length))
	      {
		w->watchpoint_triggered = watch_triggered_yes;
		break;
	      }
	  }
      }

  return 1;
}


never reaches the:
 
  w->watchpoint_triggered = watch_triggered_yes;
	
line, because this:

/* Implement the "to_watchpoint_addr_within_range" target_ops method.  */

static int
aarch64_linux_watchpoint_addr_within_range (struct target_ops *target,
					    CORE_ADDR addr,
					    CORE_ADDR start, int length)
{
  return start <= addr && start + length - 1 >= addr;
}

doesn't consider the aarch64 watchpoint alignment restrictions, and returns false if the kernel-reported stop data address is 8-byte aligned, outside the original watched range.  

So bpstat_check_watchpoint will never check the watched expression either, and thus the watchpoint trigger ends up NOT reported to the user.

Some PowerPC machines have similar restrictions, and the ppc version has this instead:

static int
ppc_linux_watchpoint_addr_within_range (...)
{
  int mask;

  if (have_ptrace_hwdebug_interface ()
      && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
    return start <= addr && start + length >= addr;
  else if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
    mask = 3;
  else
    mask = 7;

  addr &= ~mask;

  /* Check whether [start, start+length-1] intersects [addr, addr+mask].  */
  return start <= addr + mask && start + length - 1 >= addr;
}

Instead, we'll reach the moribund watchpoint locations handling, and
and automatically re-resume the program:

    fprintf_unfiltered (gdb_stdlog,
			"infrun: no user watchpoint explains "
			"watchpoint SIGTRAP, ignoring\n");


The Aarch64 watchpoint alignment restrictions are discussed here:

nat/aarch64-linux-hw-point.c:aarch64_align_watchpoint:

/* Given the (potentially unaligned) watchpoint address in ADDR and
   length in LEN, return the aligned address and aligned length in
   *ALIGNED_ADDR_P and *ALIGNED_LEN_P, respectively.  The returned
   aligned address and length will be valid values to write to the
   hardware watchpoint value and control registers.

...

   Essentially, unaligned watchpoint is achieved by minimally
   enlarging the watched area to meet the alignment requirement, and
   if necessary, splitting the watchpoint over several hardware
   watchpoint registers.  The trade-off is that there will be
   false-positive hits for the read-type or the access-type hardware
   watchpoints; for the write type, which is more commonly used, there
   will be no such issues, as the higher-level breakpoint management
   in gdb always examines the exact watched region for any content
   change, and transparently resumes a thread from a watchpoint trap
   if there is no change to the watched region.

   Another limitation is that because the watched region is enlarged,
   the watchpoint fault address returned by
   aarch64_stopped_data_address may be outside of the original watched
   region, especially when the triggering instruction is accessing a
   larger region.  When the fault address is not within any known
   range, watchpoints_triggered in gdb will get confused, as the
   higher-level watchpoint management is only aware of original
   watched regions, and will think that some unknown watchpoint has
   been triggered.  In such a case, gdb may stop without displaying
   any detailed information.

   Once the kernel provides the full support for Byte Address Select
   (BAS) in the hardware watchpoint control register, these
   limitations can be largely relaxed with some further work.  */

(...)

nat/aarch64-linux-hw-point.h:

/* Alignment requirement in bytes for addresses written to
   hardware breakpoint and watchpoint value registers.

   A ptrace call attempting to set an address that does not meet the
   alignment criteria will fail.  Limited support has been provided in
   this port for unaligned watchpoints, such that from a GDB user
   perspective, an unaligned watchpoint may be requested.

   This is achieved by minimally enlarging the watched area to meet the
   alignment requirement, and if necessary, splitting the watchpoint
   over several hardware watchpoint registers.  */

(...)
#define AARCH64_HWP_ALIGNMENT 8

For the same reason, read watchpoints on non-8-byte-aligned addresses are always missed too, instead of the occasional false positive suggested by the comments above.  I think that when that was written, GDB would stop with a spurious SIGTRAP.  Fixing the aarch64 range detection would make us report a spurious watchpoint hit, which is IMO obviously much better.

In any case, I think the occasional read watchpoint false positive is much better than ever missing (read or regular) watchpoints.
Comment 1 Pedro Alves 2016-03-10 20:01:45 UTC
This raises the question of how to fix this against gdbserver too, since 
target_watchpoint_addr_within_range / aarch64_linux_watchpoint_addr_within_range
is a target method.

I'm thinking that it might be better for the target backends to do the range / alignment checks themselves, and report a corrected stop data address to one that gdb expects, in the target_stopped_data_address hook (both native and gdbserver).
Comment 2 Pedro Alves 2016-03-10 20:02:13 UTC
TBC, I'm not working on this.
Comment 3 Jan Kratochvil 2016-06-06 08:00:31 UTC
[patch] aarch64: PR 19806: watchpoints: false negatives -> false positives
https://sourceware.org/ml/gdb-patches/2016-06/msg00078.html
Comment 4 Jan Kratochvil 2017-03-27 21:12:23 UTC
[patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
https://sourceware.org/ml/gdb-patches/2017-03/msg00470.html
Message-ID: <20170327210753.GA29656@host1.jankratochvil.net>
Comment 5 Sourceware Commits 2018-05-04 20:30:35 UTC
The master branch has been updated by Jan Kratochvil <jkratoch@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a3b60e4588606354b93508a0008a5ca04b68fad8

commit a3b60e4588606354b93508a0008a5ca04b68fad8
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Fri May 4 22:22:04 2018 +0200

    aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
    
    Some unaligned watchpoints were currently missed.
    
    On old kernels as specified in
    	kernel RFE: aarch64: ptrace: BAS: Support any contiguous range (edit)
    	https://sourceware.org/bugzilla/show_bug.cgi?id=20207
    after this patch some other unaligned watchpoints will get reported as false
    positives.
    
    With new kernels all the watchpoints should work exactly.
    
    There may be a regresion that it now less merges watchpoints so that with
    multiple overlapping watchpoints it may run out of the 4 hardware watchpoint
    registers.  But as discussed in the original thread GDB needs some generic
    watchpoints merging framework to be used by all the target specific code.
    Even current FSF GDB code does not merge it perfectly.  Also with the more
    precise watchpoints one can technically merge them less.  And I do not think
    it matters too much to improve mergeability only for old kernels.
    Still even on new kernels some better merging logic would make sense.
    
    There remains one issue:
    	kernel-4.15.14-300.fc27.armv7hl
    	FAIL: gdb.base/watchpoint-unaligned.exp: continue
    	FAIL: gdb.base/watchpoint-unaligned.exp: continue
    	(gdb) continue
    	Continuing.
    	Unexpected error setting watchpoint: Invalid argument.
    	(gdb) FAIL: gdb.base/watchpoint-unaligned.exp: continue
    But that looks as a kernel bug to me.
    (1) It is not a regression by this patch.
    (2) It is unrelated to this patch.
    
    gdb/ChangeLog
    2018-05-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
    	    Pedro Alves <palves@redhat.com>
    
    	PR breakpoints/19806 and support for PR external/20207.
    	* NEWS: Mention Aarch64 watchpoint improvements.
    	* aarch64-linux-nat.c (aarch64_linux_stopped_data_address): Fix missed
    	watchpoints and PR external/20207 watchpoints.
    	* nat/aarch64-linux-hw-point.c
    	(kernel_supports_any_contiguous_range): New.
    	(aarch64_watchpoint_offset): New.
    	(aarch64_watchpoint_length): Support PR external/20207 watchpoints.
    	(aarch64_point_encode_ctrl_reg): New parameter offset, new asserts.
    	(aarch64_point_is_aligned): Support PR external/20207 watchpoints.
    	(aarch64_align_watchpoint): New parameters aligned_offset_p and
    	next_addr_orig_p.  Support PR external/20207 watchpoints.
    	(aarch64_downgrade_regs): New.
    	(aarch64_dr_state_insert_one_point): New parameters offset and
    	addr_orig.
    	(aarch64_dr_state_remove_one_point): Likewise.
    	(aarch64_handle_breakpoint): Update caller.
    	(aarch64_handle_aligned_watchpoint): Likewise.
    	(aarch64_handle_unaligned_watchpoint): Support addr_orig and
    	aligned_offset.
    	(aarch64_linux_set_debug_regs): Remove const from state.  Call
    	aarch64_downgrade_regs.
    	(aarch64_show_debug_reg_state): Print also dr_addr_orig_wp.
    	* nat/aarch64-linux-hw-point.h (DR_CONTROL_LENGTH): Rename to ...
    	(DR_CONTROL_MASK): ... this.
    	(struct aarch64_debug_reg_state): New field dr_addr_orig_wp.
    	(unsigned int aarch64_watchpoint_offset): New prototype.
    	(aarch64_linux_set_debug_regs): Remove const from state.
    	* utils.c (align_up, align_down): Move to ...
    	* common/common-utils.c (align_up, align_down): ... here.
    	* utils.h (align_up, align_down): Move to ...
    	* common/common-utils.h (align_up, align_down): ... here.
    
    gdb/gdbserver/ChangeLog
    2018-05-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
    	    Pedro Alves <palves@redhat.com>
    
    	* linux-aarch64-low.c (aarch64_stopped_data_address):
    	Likewise.
    
    gdb/testsuite/ChangeLog
    2018-05-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
    	    Pedro Alves <palves@redhat.com>
    
    	PR breakpoints/19806 and support for PR external/20207.
    	* gdb.base/watchpoint-unaligned.c: New file.
    	* gdb.base/watchpoint-unaligned.exp: New file.
Comment 6 Jan Kratochvil 2018-05-04 20:31:36 UTC
Fixed.