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

[Pedro Alves <palves at redhat dot com>]

On 06/08/2016 05:42 PM, Yao Qi wrote:
> 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.
> 

But this way, once the kernel is fixed, we'll continue reporting false
positives for read watchpoints.  And we'll unnecessarily report false
positives with any emulator that doesn't have this restriction (think
Valgrind, etc.).

How is this better than making it the responsibility of the
stub to report a stop data address that is within the address
range that gdb requested to watch, in the first place?

By making it the target's responsibility, the false positives will
disappear once the kernel (and the target) is fixed.  With a gdbarch
approach, we will always have false positives, as long as we
support older kernels.  And as long as we carry the workaround,
we will always have false positives against simulators/emulators
that really only trap the addresses gdb requested.

> 
>> 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.

Only for regular watchpoints...  Read/access watchpoints are not
so lucky.

> 
>>
>>>
>>>> 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.

No, because a gdbarch method can't tell whether have_ptrace_hwdebug_interface()
returns true.

> 
>>
>> (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.

Only if you ignore read watchpoints.  But those are real things.  We
shouldn't ignore them.

> 
>> 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.

I'm saying that even if a machine/cpu has some restriction that only
8-byte aligned addresses can be watched, a stub is free to implement
watchpoints some other way that gets over that limitation.  By hardcoding
the alignment assumptions in gdb, you're setting up for false positives
which could be entirely avoided if gdb wasn't in the business of
hardcoding them.

> 
>>
>> 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.
> 

They _are_ harmful, because they prevent fixing read watchpoint false
positives.

Thanks,
Pedro Alves