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

[Yao Qi <qiyaoltc@gmail.com>]

Pedro Alves <palves@redhat.com> writes:

> How do you plan on handling remote targets though?  Done that way, it 
> sounds to me like the alignment restrictions should either be a gdbarch
> property, or you need some RSP extension, e.g., extend the "watch" stop
> reply to indicate an stop data address range instead of a sole address,
> or make the stub report back the alignment restriction when GDB tells it
> to insert the watchpoint in the first place, instead of just saying
> "OK".
>

I want to use a gdbarch method to decide whether a watchpoint is
triggered in remote target, like this, [note that all the code below is
not compiled at all]

static enum watchpoint_triggered
remote_watchpoint_triggered (struct target_ops *target, struct watchpoint *w)
{
  struct thread_info *thread = inferior_thread ();

  if (thread->priv != NULL
      && thread->priv->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
    {
      return gdbarch_watchpoint_triggered (w->base.gdbarch, w,
					   thread->priv->watch_data_address);
    }

  return watchpoint_triggered_unknown;
}

The default implementation of gdbarch_watchpoint_triggered returns
watchpoint_triggered_yes if address is in the range of
loc->address,+loc->length.  Otherwise return watchpoint_triggered_no.
Then, we can rewrite breakpoint.c:watchpoints_triggered like this,

int
watchpoints_triggered (void)
{
  int ret = 0;

  ALL_BREAKPOINTS (b)
    if (is_hardware_watchpoint (b))
      {
	struct watchpoint *w = (struct watchpoint *) b;

	w->watchpoint_triggered = target_watchpoint_triggered (w);

	if (w->watchpoint_triggered != watch_triggered_no)
	  ret = 1;
      }

  return ret;
}

> A gdbarch method poses problems for remote stubs that are actually emulators,
> and thus can support hardware watchpoints without these restrictions.

Then, the address reported by the target should fall in the range of
loc->address,+loc->length.

> I think it's actually problematic for real machines, as the restrictions
> will often depend on process revisions/models.  So a gdbarch approach
> would be undesirable, IMO.

On the real machine, nowadays, the restriction is that address must be
8-byte-aligned on linux.  The restriction can only be relaxed and
may be removed finally in the future, IOW, the restriction won't become
16-byte aligned, so we can write the gdbarch method for aarch64-linux
like this,

static enum watchpoint_triggered
aarch64_linux_watchpoint_triggered (struct gdbarch *gdbarch,
				    struct watchpoint *w,
				    CORE_ADDR addr)
{
  struct bp_location *loc;

  for (loc = w->base.loc; loc; loc = loc->next)
    {
      if (addr >= loc->address && addr < loc->address + loc->length)
	{
	  /* If the address reported by the target falls in the memory
	     range that watchpoint W is monitoring, the watchpoint is
	     definitely hit.  */
	  return watchpoint_triggered_yes;
	}
      else if (addr >= align_down (loc->address, 8) && addr < loc->address)
	{
	  /* The debugging stub, like GDBserver, may adjust the address
	     to meet kernel's alignment requirements (8-aligned for
	     example), we are not sure watchpoint W is hit or not.  */
	  return watchpoint_triggered_unknown;
	}
    }

  return watchpoint_triggered_no;
}

-- 
Yao (齐尧)