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

Yao Qi
2016-06-08T16:42:27Z

Pedro Alves <palves@redhat.com> writes:

> In any case, with the gdbarch method, if the target supports watching all
> kinds of unaligned addresses/ranges, and reports the correct address in
> "T05 watch" stop replies, it then faces the problem that gdb has hardcoded
> knowledge of watch region alignment restrictions, and then e.g., watchpoints
> on consecutive addresses are misinterpreted, even though there's
> no reason for that.
>

The gdbarch method should first check if reported address is within the
range of watchpoint location, if yes, return watch_triggered_yes, tell
GDB that the watchpoint is definitely hit.

If we can't find any one range of watchpoint location that the report
address is within, we'll guess that the target may be monitoring aligned
address. We check watchpoint locations again, to see if address is
within [align_down (loc->address, 8), loc->address], if yes, return
watch_triggered_unknown, tell GDB that watchpoint is hit, but target
doesn't tell the address. [note that we'll miss read watchpoint in this
approach]. If no luck, return watch_triggered_no.

static enum watchpoint_triggered
aarch64_linux_watchpoint_triggered (struct gdbarch *gdbarch, CORE_ADDR addr,
struct breakpoint *b)
{
struct bp_location *loc;
int unknown = 0;

for (loc = b->loc; loc != NULL; 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 watch_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. */
unknown = 1;
}
}

if (unknown)
return watch_triggered_unknown;
else
return watch_triggered_no;
}

> E.g., say gdb believes the machine 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[1], and the target reports
> a memory access to 'global.buf + 1', gdb will believe that
> watchpoint 2 _also_ triggered, when it did not. That's a false positive you
> can't help with with real machines, but there's no reason a
> simulator/emulator has to suffer from that.

As I described above, gdbarch method will return watch_triggered_yes for
watchpoint 1, and watch_triggered_unknown for watchpoint 2. After GDB
checks the value change, it ignores the latter, which is good. The
false positive can be eliminated.

>
>>
>>> 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.
>
> I think we need to consider all architectures, and the design going
> forward, not just Aarch64. For example, PPC has:

We put things into gdbarch method, which is to handle the differences of
different archs.

>
> static int
> ppc_linux_watchpoint_addr_within_range (struct target_ops *target,
> CORE_ADDR addr,
> CORE_ADDR start, int length)
> {
> 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;
>
> So e.g., here, the alignment restrictions depend on both
> the processor model and kernel.

This can be in the gdbarch method for ppc-linux.

>
> (I'll guess that other embedded architectures that gdb supports probably
> have similar restrictions that gdb was never taught about.)
>

Yes, but we should teach GDB to do what we already know of.

>> 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,
>
> How can gdb determine whether the restriction has been lifted?

In the step of checking whether a given address is within the range
monitored by watchpoint, gdb doesn't need to determine that. Once the
restriction is removed, the address reported by the target is exactly
the address of watchpoint, the gdbarch method returns watch_triggered_yes.

> The way to do it is probably either by checking kernel version or having the
> ptrace code that "inserts" the watchpoint to first try watching the unaligned
> region exactly as gdb requested, and if that doesn't work, try a wider,
> aligned region. Only that target-side ptrace code is aware of these
> finer details
> and the correct restrictions in effect for the running system. If we put this
> in a gdbarch method, how can the gdbarch method maintain compatibility with
> older kernels and at the same time reflect that newer kernels no longer
> impose the restriction?

The gdbarch method only does some complementary guess if reported
address doesn't fall in the range of watchpoint locations. If there is
no such restriction, the guess is not used at all.

>
> It's actually not just simulators/emulators that can have different
> watchpoint restrictions from the machine architecture's debug hardware
> limitations. This is in good part a debug API issue as well --
> a target may well support watchpoints implemented in a totally different
> way -- for example, I believe Solaris supports "unlimited" watchpoints and
> address ranges by implementing watchpoints not by using debug registers, but
> instead by changing memory page protections and trapping faults internally,
> all invisibly to userspace.

If they are invisible to user space, we can do nothing, but I don't know
how is it related to this issue.

>
> All this is why I believe that hardcoding this knowledge in gdb, which is
> what a gdbarch method does, is not the best approach.

These knowledge is only used when there are some limitations in the
debugging api, and these knowledge are not harmful once the limitations
are fixed.

--
Yao (齐尧)