[patch] pr11371 conditional watchpoints with a function in the condition.

[patch] pr11371 conditional watchpoints with a function in the condition.

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 217 bytes --]

The description in this PR doesn't describe thing well--what's really 
happening is that if the condition associated with a watchpoint 
contained a function call, it resulted in a segfault.  This patch fixes 
that.



[-- Attachment #2: pr11371.patch --]
[-- Type: text/x-patch, Size: 697 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.486
diff -u -r1.486 breakpoint.c
--- breakpoint.c	13 May 2010 22:44:02 -0000	1.486
+++ breakpoint.c	2 Jun 2010 19:39:12 -0000
@@ -1355,7 +1355,10 @@
   /* We don't free locations.  They are stored in bp_location array and
      update_global_locations will eventually delete them and remove
      breakpoints if needed.  */
-  b->loc = NULL;
+  
+  if (b->type != bp_watchpoint && b->type != bp_hardware_watchpoint &&
+      b->enable_state != bp_call_disabled)
+    b->loc = NULL;
 
   if (b->disposition == disp_del_at_next_stop)
     return;

[-- Attachment #3: pr11371-sum.diff --]
[-- Type: text/x-patch, Size: 3329 bytes --]

--- virgin.sum	2010-06-04 12:39:10.180908008 -0400
+++ patched.sum	2010-06-04 13:13:18.561033412 -0400
@@ -1,4 +1,4 @@
-Test Run By moller on Fri Jun  4 12:24:14 2010
+Test Run By moller on Fri Jun  4 13:05:22 2010
 Native configuration is i686-pc-linux-gnu
 
 		=== gdb tests ===
@@ -6047,6 +6047,11 @@
 PASS: gdb.base/pr11022.exp: breakpoint hit 2
 PASS: gdb.base/pr11022.exp: set var x = 1
 PASS: gdb.base/pr11022.exp: watchpoint hit 2
+Running ../../../src/gdb/testsuite/gdb.base/pr11371.exp ...
+PASS: gdb.base/pr11371.exp: watch aa if cond(aa)
+PASS: gdb.base/pr11371.exp: run to conditional watchpoint
+PASS: gdb.base/pr11371.exp: run to conditional watchpoint
+PASS: gdb.base/pr11371.exp: run to conditional watchpoint
 Running ../../../src/gdb/testsuite/gdb.base/prelink.exp ...
 PASS: gdb.base/prelink.exp: set verbose on
 PASS: gdb.base/prelink.exp: prelink
@@ -14384,7 +14389,7 @@
 Running ../../../src/gdb/testsuite/gdb.mi/mi2-console.exp ...
 PASS: gdb.mi/mi2-console.exp: breakpoint at main
 PASS: gdb.mi/mi2-console.exp: mi runto main
-FAIL: gdb.mi/mi2-console.exp: Started step over hello
+PASS: gdb.mi/mi2-console.exp: Started step over hello
 KFAIL: gdb.mi/mi2-console.exp: Hello message (PRMS: gdb/623)
 PASS: gdb.mi/mi2-console.exp: finished step over hello
 Running ../../../src/gdb/testsuite/gdb.mi/mi2-disassemble.exp ...
@@ -16222,8 +16227,8 @@
 PASS: gdb.threads/attachstop-mt.exp: attach4 to stopped switch thread
 PASS: gdb.threads/attachstop-mt.exp: attach4 to stopped bt
 PASS: gdb.threads/attachstop-mt.exp: continue (attach4 continue)
-PASS: gdb.threads/attachstop-mt.exp: attach4 stop by interrupt
-PASS: gdb.threads/attachstop-mt.exp: attach4, exit leaves process sleeping
+FAIL: gdb.threads/attachstop-mt.exp: attach4 stop by interrupt (timeout)
+FAIL: gdb.threads/attachstop-mt.exp: attach4, exit leaves process sleeping
 Running ../../../src/gdb/testsuite/gdb.threads/bp_in_thread.exp ...
 PASS: gdb.threads/bp_in_thread.exp: successfully compiled posix threads test case
 PASS: gdb.threads/bp_in_thread.exp: breakpoint on noreturn
@@ -16542,7 +16547,7 @@
 PASS: gdb.threads/schedlock.exp: step without lock does not change thread
 PASS: gdb.threads/schedlock.exp: listed args (3)
 PASS: gdb.threads/schedlock.exp: current thread stepped
-FAIL: gdb.threads/schedlock.exp: other threads ran - unlocked
+PASS: gdb.threads/schedlock.exp: other threads ran - unlocked
 PASS: gdb.threads/schedlock.exp: set scheduler-locking on
 PASS: gdb.threads/schedlock.exp: continue (with lock)
 PASS: gdb.threads/schedlock.exp: stop all threads (with lock)
@@ -16753,7 +16758,7 @@
 PASS: gdb.threads/watchthreads2.exp: all threads started
 PASS: gdb.threads/watchthreads2.exp: watch x
 PASS: gdb.threads/watchthreads2.exp: set var test_ready = 1
-KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in multithreaded app (PRMS: gdb/10116)
+PASS: gdb.threads/watchthreads2.exp: all threads incremented x
 Running ../../../src/gdb/testsuite/gdb.trace/actions.exp ...
 PASS: gdb.trace/actions.exp: 5.1a: set three tracepoints, no actions
 PASS: gdb.trace/actions.exp: 5.1b: set actions for first tracepoint
@@ -17019,7 +17024,7 @@
 
 		=== gdb Summary ===
 
-# of expected passes		16154
+# of expected passes		16159
 # of unexpected failures	31
 # of expected failures		47
 # of untested testcases		7

Re: [patch] pr11371 conditional watchpoints with a function in the condition.

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

Sorry about that...

cm

On 06/04/10 16:14, Jan Kratochvil wrote:
> On Fri, 04 Jun 2010 19:24:05 +0200, Chris Moller wrote:
>    
>> +  if (b->type != bp_watchpoint&&  b->type != bp_hardware_watchpoint&&
>> +      b->enable_state != bp_call_disabled)
>>      
>
> `&&' should be on the second line.
> 	http://www.gnu.org/prep/standards/standards.html#Formatting
> 	When you split an expression into multiple lines, split it before an
> 	operator, not after one.
>
>
>    
>> +Running ../../../src/gdb/testsuite/gdb.base/pr11371.exp ...
>> +PASS: gdb.base/pr11371.exp: watch aa if cond(aa)
>> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint
>> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint
>> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint
>>      
>
> Apparently you have written a new testcase but forgot to attach it.
> I have not tried to do real a review without the testcase.
>
>
> Thanks,
> Jan
>    


[-- Attachment #2: pr11371.exp --]
[-- Type: text/plain, Size: 1454 bytes --]

# Copyright 2010 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, see <http://www.gnu.org/licenses/>.

#
# Tests watchpoints that watch expressions that don't involve memory.
#

set testfile "pr11371"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}

if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
    untested pr11371.exp
    return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

if ![runto_main] then {
    fail "Can't run to main"
    return 0
}

gdb_test "watch aa if cond(aa)"

gdb_test "run" "aa = 5;.*" "run to conditional watchpoint" "Start it from the beginning.*" "y"
gdb_test "run" "aa = 5;.*" "run to conditional watchpoint" "Start it from the beginning.*" "y"
gdb_test "run" "aa = 5;.*" "run to conditional watchpoint" "Start it from the beginning.*" "y"


[-- Attachment #3: pr11371.c --]
[-- Type: text/x-csrc, Size: 852 bytes --]

/* This test is part of GDB, the GNU debugger.

   Copyright 2010 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, see <http://www.gnu.org/licenses/>.
   */

int aa = 4;

int
cond(int a)
{
  return a == 6;
}

main()
{
  aa = 5;
  aa = 6;
  aa = 7;

  return 0;
}

Re: [patch] pr11371 conditional watchpoints with a function in the condition.

[Jan Kratochvil]

On Fri, 04 Jun 2010 19:24:05 +0200, Chris Moller wrote:
> +  if (b->type != bp_watchpoint && b->type != bp_hardware_watchpoint &&
> +      b->enable_state != bp_call_disabled)

`&&' should be on the second line.
	http://www.gnu.org/prep/standards/standards.html#Formatting
	When you split an expression into multiple lines, split it before an
	operator, not after one.


> +Running ../../../src/gdb/testsuite/gdb.base/pr11371.exp ...
> +PASS: gdb.base/pr11371.exp: watch aa if cond(aa)
> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint
> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint
> +PASS: gdb.base/pr11371.exp: run to conditional watchpoint

Apparently you have written a new testcase but forgot to attach it.
I have not tried to do real a review without the testcase.


Thanks,
Jan

Re: [patch] pr11371 conditional watchpoints with a function in the condition.

[Jan Kratochvil]

On Fri, 04 Jun 2010 19:24:05 +0200, Chris Moller wrote:
> if the condition associated with a
> watchpoint contained a function call, it resulted in a segfault.

> --- breakpoint.c	13 May 2010 22:44:02 -0000	1.486
> +++ breakpoint.c	2 Jun 2010 19:39:12 -0000
> @@ -1355,7 +1355,10 @@
>    /* We don't free locations.  They are stored in bp_location array and
>       update_global_locations will eventually delete them and remove
>       breakpoints if needed.  */
> -  b->loc = NULL;
> +  
> +  if (b->type != bp_watchpoint && b->type != bp_hardware_watchpoint &&
> +      b->enable_state != bp_call_disabled)
> +    b->loc = NULL;

This change has a regression for:
int a[1000], *p = a;
int
main (void)
{
  int i;
  for (i = 0; i < 100; i++)
    p++;
  return 0;
}

./gdb -nx -ex 'watch *p' -ex r ./pr11371regression
	Watchpoint 1: *p
	Starting program: .../pr11371regression 
	Program exited normally.
->
	Watchpoint 1: *p
	Starting program: .../pr11371regression 
	Warning:
	Could not insert hardware watchpoint 1.
	Could not insert hardware breakpoints:
	You may have requested too many hardware breakpoints/watchpoints.
	(gdb) p i
	$1 = 2

The crash happened due to referencing unallocated memory of a freed
bp_location.  Your patch avoids freeing bp_locations in some cases.  But it is
not correct as these bp_locations become superfluous causing needless hardware
watchpoints.

I believe this `b->loc = NULL' should remain in place, there is more a problem
of bpstat not linked to thread should get cleared its stale bp_location
reference.

I was thinking more about something around the patch below but it does not
work anyway so just posting FYI.  Also maybe on should pre-apply:
	[patch 1/3] Clear stale specific locs, not whole bpts [rediff]
	http://sourceware.org/ml/gdb-patches/2010-05/msg00366.html


BTW the testcase gdb.base/pr11371.exp PASSes for me even on FSF GDB HEAD.


Thanks,
Jan


--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4108,6 +4111,9 @@ bpstat_stop_status (struct address_space *aspace,
 	  /* Print nothing for this entry if we dont stop or dont print.  */
 	  if (bs->stop == 0 || bs->print == 0)
 	    bs->print_it = print_it_noop;
+
+	  if (b->type == bp_hardware_watchpoint)
+	    break;
 	}
     }