[patch] Fix crash on conditional watchpoints (PR 11371)

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi,

on a watchpoint with condition using inferior function call GDB crashes.

== Invalid read of size 8
==    at 0x63949B: bpstat_stop_status (breakpoint.c:4138)
==    by 0x69C145: handle_inferior_event (infrun.c:3873)
==    by 0x6993C4: wait_for_inferior (infrun.c:2552)
==    by 0x698680: proceed (infrun.c:2065)
==    by 0x69144A: run_command_1 (infcmd.c:586)
==    by 0x691484: run_command (infcmd.c:596)
==    by 0x5F0934: do_cfunc (cli-decode.c:67)
==    by 0x5F39A2: cmd_func (cli-decode.c:1771)
==    by 0x48A81D: execute_command (top.c:422)
==    by 0x6AC9D4: catch_command_errors (exceptions.c:534)
==    by 0x4811E3: captured_main (main.c:887)
==    by 0x6AC931: catch_errors (exceptions.c:518)
==    by 0x48127E: gdb_main (main.c:919)
==    by 0x47FF15: main (gdb.c:34)
==  Address 0xcc727b0 is 16 bytes inside a block of size 184 free'd
==    at 0x4C25D72: free (vg_replace_malloc.c:325)
==    by 0x48E503: xfree (utils.c:1505)
==    by 0x63B9CA: free_bp_location (breakpoint.c:5402)
==    by 0x643B54: update_global_location_list (breakpoint.c:9428)
==    by 0x635AAD: insert_breakpoints (breakpoint.c:1867)
==    by 0x69845E: proceed (infrun.c:1985)
==    by 0x68F950: run_inferior_call (infcall.c:378)
==    by 0x6904AF: call_function_by_hand (infcall.c:804)
==    by 0x65BD6B: evaluate_subexp_standard (eval.c:1794)
==    by 0x73D1AA: evaluate_subexp_c (c-lang.c:1047)
==    by 0x65780A: evaluate_subexp (eval.c:76)
==    by 0x657A09: evaluate_expression (eval.c:167)
==    by 0x6382F8: breakpoint_cond_eval (breakpoint.c:3437)
==    by 0x6AC931: catch_errors (exceptions.c:518)
==    by 0x63902B: bpstat_check_breakpoint_conditions (breakpoint.c:3970)
==    by 0x63924B: bpstat_stop_status (breakpoint.c:4082)
==    by 0x69C145: handle_inferior_event (infrun.c:3873)
==    by 0x6993C4: wait_for_inferior (infrun.c:2552)
==    by 0x698680: proceed (infrun.c:2065)
==    by 0x69144A: run_command_1 (infcmd.c:586)
==    by 0x691484: run_command (infcmd.c:596)
==    by 0x5F0934: do_cfunc (cli-decode.c:67)
==    by 0x5F39A2: cmd_func (cli-decode.c:1771)
==    by 0x48A81D: execute_command (top.c:422)
==    by 0x6AC9D4: catch_command_errors (exceptions.c:534)

It is due to bpstat_stop_status calling bpstat_check_breakpoint_conditions
which calls update_global_location_list which rebuilds bp_location's which
bpstat_stop_status does not expect.

I tried first to fill in bpstat_stop_status the bpstat events already into
ecs->event_thread->stop_bpstat so that free_bp_location would find it there
and clear it appropriately.  It does not work as in the time free_bp_location
gets called ecs->event_thread->stop_bpstat is already pre-cleared and there is
no way for free_bp_location to find the bpstat_what list stored only in the
local variable in bpstat_stop_status.

With the fix it just unfortunately does not stop when it should.  This is due
to all-stop mode always removing and re-creating watchpoint locations and
bpstat_what() never stops on `bs->breakpoint_at == NULL'.  Anyway non-stop
mode should solve it as it no longer re-creates watchpoints each time.
Unfortunately I could not verify it as non-stop fails for me in this case in
some general way.  I would turn the PR from a crashing one into a non-working
one, there is a KFAIL in the testcase for it below.  I believe the problem it
does not stop is unrelated to this one.

No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.


Thanks,
Jan


gdb/
2010-08-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bpstat_remove_bp_location): New forward declaration.
	(bpstat_pending, bpstat_pending_link): New variables.
	(bpstat_alloc): Remove parameter bs_link_pointer.  Use
	bpstat_pending_link instead.
	(bpstat_stop_status_cleanup): New function.
	(bpstat_stop_status): Remove variable bs_link.  New variable
	save_bpstat_pending_link.  New variable back_to, wrap the function by
	it.  Move the bp_hardware_watchpoint type check into the initial loop
	statement.  Adjust the calls of bpstat_alloc, initialize bs_head
	explicitly there.  Add extra check for NULL bs->breakpoint_at.
	(free_bp_location): Check also bpstat_pending.

gdb/testsuite/
2010-08-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watch-cond-by-func.exp: New file.
	* gdb.base/watch-cond-by-func.c: New file.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -207,6 +207,8 @@ static void update_global_location_list (int);
 
 static void update_global_location_list_nothrow (int);
 
+static void bpstat_remove_bp_location (bpstat bps, struct bp_location *loc);
+
 static int bpstat_remove_bp_location_callback (struct thread_info *th,
 					       void *data);
 
@@ -3440,17 +3442,21 @@ breakpoint_cond_eval (void *exp)
   return i;
 }
 
+/* List of bpstat's to be checked for stale information.  */
+
+static bpstat bpstat_pending, *bpstat_pending_link = &bpstat_pending;
+
 /* Allocate a new bpstat.  Link it to the FIFO list by BS_LINK_POINTER.  */
 
 static bpstat
-bpstat_alloc (const struct bp_location *bl, bpstat **bs_link_pointer)
+bpstat_alloc (const struct bp_location *bl)
 {
   bpstat bs;
 
   bs = (bpstat) xmalloc (sizeof (*bs));
   bs->next = NULL;
-  **bs_link_pointer = bs;
-  *bs_link_pointer = &bs->next;
+  *bpstat_pending_link = bs;
+  bpstat_pending_link = &bs->next;
   bs->breakpoint_at = bl;
   /* If the condition is false, etc., don't do the commands.  */
   bs->commands = NULL;
@@ -4002,6 +4008,17 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
     }
 }
 
+/* Cleanup helper for bpstat_stop_status.  */
+
+static void
+bpstat_stop_status_cleanup (void *arg)
+{
+  bpstat *link = arg;
+
+  gdb_assert (*bpstat_pending_link == NULL);
+  bpstat_clear (link);
+  bpstat_pending_link = link;
+}
 
 /* Get a bpstat associated with having just stopped at address
    BP_ADDR in thread PTID.
@@ -4028,11 +4045,15 @@ bpstat_stop_status (struct address_space *aspace,
   struct bp_location *bl;
   struct bp_location *loc;
   /* First item of allocated bpstat's.  */
-  bpstat bs_head = NULL, *bs_link = &bs_head;
+  bpstat bs_head = NULL;
   /* Pointer to the last thing in the chain currently.  */
   bpstat bs;
+  bpstat *save_bpstat_pending_link = bpstat_pending_link;
   int ix;
   int need_remove_insert;
+  struct cleanup *back_to;
+  
+  back_to = make_cleanup (bpstat_stop_status_cleanup, bpstat_pending_link);
 
   /* ALL_BP_LOCATIONS iteration would break across
      update_global_location_list possibly executed by
@@ -4043,16 +4064,15 @@ bpstat_stop_status (struct address_space *aspace,
       if (!breakpoint_enabled (b) && b->enable_state != bp_permanent)
 	continue;
 
-      for (bl = b->loc; bl != NULL; bl = bl->next)
+      for (bl = b->loc; bl != NULL;
+	   /* For hardware watchpoints, we look only at the first location.
+	      The watchpoint_check function will work on the entire
+	      expression, not the individual locations.  For read watchpoints,
+	      the watchpoints_triggered function has checked all locations
+	      already.  Also *BL can become no longer valid during the
+	      bpstat_check_breakpoint_conditions call so be careful.  */
+           bl = b->type == bp_hardware_watchpoint ? NULL : bl->next)
 	{
-	  /* For hardware watchpoints, we look only at the first location.
-	     The watchpoint_check function will work on the entire expression,
-	     not the individual locations.  For read watchpoints, the
-	     watchpoints_triggered function has checked all locations
-	     already.  */
-	  if (b->type == bp_hardware_watchpoint && bl != b->loc)
-	    break;
-
 	  if (bl->shlib_disabled)
 	    continue;
 
@@ -4061,7 +4081,9 @@ bpstat_stop_status (struct address_space *aspace,
 
 	  /* Come here if it's a watchpoint, or if the break address matches */
 
-	  bs = bpstat_alloc (bl, &bs_link);	/* Alloc a bpstat to explain stop */
+	  bs = bpstat_alloc (bl);	/* Alloc a bpstat to explain stop */
+	  if (bs_head == NULL)
+	    bs_head = bs;
 
 	  /* Assume we stop.  Should we find watchpoint that is not actually
 	     triggered, or if condition of breakpoint is false, we'll reset
@@ -4119,7 +4141,10 @@ bpstat_stop_status (struct address_space *aspace,
       if (breakpoint_address_match (loc->pspace->aspace, loc->address,
 				    aspace, bp_addr))
 	{
-	  bs = bpstat_alloc (loc, &bs_link);
+	  bs = bpstat_alloc (loc);
+	  if (bs_head == NULL)
+	    bs_head = bs;
+
 	  /* For hits of moribund locations, we should just proceed.  */
 	  bs->stop = 0;
 	  bs->print = 0;
@@ -4135,6 +4160,7 @@ bpstat_stop_status (struct address_space *aspace,
   if (! bpstat_causes_stop (bs_head))
     for (bs = bs_head; bs != NULL; bs = bs->next)
       if (!bs->stop
+	  && bs->breakpoint_at
 	  && bs->breakpoint_at->owner
 	  && is_hardware_watchpoint (bs->breakpoint_at->owner))
 	{
@@ -4148,6 +4174,13 @@ bpstat_stop_status (struct address_space *aspace,
   if (need_remove_insert)
     update_global_location_list (1);
 
+  /* Break the chain and make BS_HEAD independent.  */
+  discard_cleanups (back_to);
+  gdb_assert (*bpstat_pending_link == NULL);
+  bpstat_pending_link = save_bpstat_pending_link;
+  gdb_assert (*bpstat_pending_link == bs_head);
+  *bpstat_pending_link = NULL;
+
   return bs_head;
 }
 
@@ -5394,6 +5427,8 @@ static void free_bp_location (struct bp_location *loc)
 
   iterate_over_threads (bpstat_remove_bp_location_callback, loc);
 
+  bpstat_remove_bp_location (bpstat_pending, loc);
+
   if (loc->cond)
     xfree (loc->cond);
 
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-cond-by-func.c
@@ -0,0 +1,33 @@
+/* This testcase 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/>.  */
+
+volatile int var;
+
+int
+return_1 (void)
+{
+  return 1;
+}
+
+int
+main(void)
+{
+  var++;
+  var++;	/* watchpoint-stop */
+
+  return 0;	/* break-at-exit */
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-cond-by-func.exp
@@ -0,0 +1,43 @@
+# 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/>.
+
+set testfile "watch-cond-by-func"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+gdb_test "watch var if return_1 ()" "atchpoint .*: var"
+
+gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+
+set test "continue"
+gdb_test_multiple $test $test {
+    -re "atchpoint \[0-9\]+: var\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*watchpoint-stop.*.*\r\n$gdb_prompt $" {
+	pass $test
+    }
+    -re "break-at-exit.*\r\n$gdb_prompt $" {
+	# It should have stopped.
+	kfail breakpoint/11371 $test
+    }
+}