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

[Yao Qi <qiyaoltc at gmail dot com>]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Hi Jan,

> Aarch64: watchpoints set on non-8-byte-aligned addresses are always missed
> https://sourceware.org/bugzilla/show_bug.cgi?id=19806
>
> some unaligned watchpoints are currently missed.  After this patch some other
> unaligned watchpoints will get reported as false positives.
>
> It could be probably all fixed on the kernel side, filed a RFE for it:
> 	kernel RFE: aarch64: ptrace: BAS: Support any contiguous range
> 	https://sourceware.org/bugzilla/show_bug.cgi?id=20207
> ->
> 	https://bugzilla.redhat.com/show_bug.cgi?id=1342821
> I have no idea if/when the kernel part gets fixed so I am posting this
> hopefully-temporary GDB change.
>

I agree that false positives are better than false negatives.

I don't think this problem is aarch64-linux specific, because other
targets, such as mips and ppc have the similar restrictions.  IMO, we
need to fix it in a target-independent way.  I've had some ideas, but
still need some experiments to see how good/bad they are.

In general, the trouble maker is target_stopped_data_address in
watchpoints_triggered, because it returns the 8-byte-aligned address,
which doesn't falls in the range of [loc->address, loc->address + loc->length).
However, watchpoints_triggered calls three target hooks and does two
things,

 - return true if target is stopped by watchpoints, and return false
   otherwise,
 - update watchpoint.watchpoint_triggered,

this leads me thinking that why do we need to get "inaccurate address"
from target_stopped_data_address, and pass it to
target_watchpoint_addr_within_range.  Instead, we can pass the
watchpoint to the (new) target hook, and set
watchpoint.watchpoint_triggered in different target implementations.  In
each target implementation, we can set .watchpoint_triggered to
watch_triggered_{yes,no,unknown} according to its hardware feature or
capability.

I'll give a try this way.

> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> new file mode 100644
> index 0000000..623314a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -0,0 +1,82 @@
> +# Copyright 2016 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> +#
> +# This file is part of the gdb testsuite.
> +
> +if {![is_aarch64_target] && ![istarget "x86_64-*-*"] && ![istarget i?86-*]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}

Can we generalize this test to any targets supports watchpoint?

> +
> +standard_testfile
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +gdb_breakpoint "[gdb_get_line_number "again_start"]" "Breakpoint $decimal at $hex" "again_start"
> +
> +set sizes {1 2 4 8}
> +array set alignedend {1 1  2 2  3 4  4 4  5 8  6 8  7 8  8 8}
> +
> +foreach wpsize $sizes {
> +    for {set wpoffset 0} {$wpoffset < 8/$wpsize} {incr wpoffset} {
> +	set wpstart [expr $wpoffset * $wpsize]
> +	set wpend [expr ($wpoffset + 1) * $wpsize]
> +	set wpendaligned $alignedend($wpend)
> +	foreach rdsize $sizes {
> +	    for {set rdoffset 0} {$rdoffset < 8/$rdsize} {incr rdoffset} {
> +		set rdstart [expr $rdoffset * $rdsize]
> +		set rdend [expr ($rdoffset + 1) * $rdsize]
> +		set expect_hit [expr max($wpstart,$rdstart) < min($wpend,$rdend)]
> +		set test "rwatch data.u.size$wpsize\[$wpoffset\]"

Can we test both read watch and write watch?  Secondly, can you add some
comments on the code?

> +		set wpnum ""
> +		gdb_test_multiple $test $test {
> +		    -re "Hardware read watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
> +			set wpnum $expect_out(1,string)
> +		    }
> +		}
> +		gdb_test_no_output "set variable size = $rdsize" ""
> +		gdb_test_no_output "set variable offset = $rdoffset" ""
> +		set test "continue"
> +		set did_hit 0
> +		gdb_test_multiple $test $test {
> +		    -re "Hardware read watchpoint $wpnum:.*Value = .*\r\n$gdb_prompt $" {
> +			set did_hit 1
> +			send_gdb "continue\n"
> +			exp_continue
> +		    }
> +		    -re " again_start .*\r\n$gdb_prompt $" {
> +		    }
> +		}
> +		gdb_test_no_output "delete $wpnum" ""
> +		set test "wp(size=$wpsize offset=$wpoffset) rd(size=$rdsize offset=$rdoffset) expect=$expect_hit"
> +		if {$expect_hit == 0 && $rdstart < $wpendaligned} {
> +		    setup_kfail external/20207 "aarch64-*-*"

It should be xfail rather than kfail.

-- 
Yao (éå)