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

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

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;
 	}
     }