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

Yao Qi
2016-06-07T13:23:55Z

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 (齐尧)