Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [rfc] Allow watchpoints on inaccessible memory
@ 2007-08-21 14:25 Daniel Jacobowitz
  2007-08-21 16:33 ` Jim Blandy
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Daniel Jacobowitz @ 2007-08-21 14:25 UTC (permalink / raw)
  To: gdb-patches

Here's something I've been meaning to try for ages.  Suppose you have
a global variable pointing to some dynamically allocated storage, and
you want to find writes into that storage.  When the pointer isn't
initialized you can't even set the watchpoint:

#include <stdlib.h>

char *ptr;

int
main ()
{
  ptr = malloc (65536);

  ptr[32768] = 1;
}

(gdb) watch ptr[32768]
Cannot access memory at address 0x8000

When it is initialized you can, but if you re-run then GDB falls over:

(gdb) r
Starting program: /space/fsf/watch
Error in re-setting breakpoint 2:
Cannot access memory at address 0x8000
Cannot access memory at address 0x8000

This limitation is unnecessary.  Hardware watchpoint registers work
by trapping actual write operations; they don't (usually anyway)
require access to the actual contents.  If we treat "inaccessible" as
a sentinel value not equal to any other value, we can set the
watchpoint at any time.

(gdb) r
Starting program: /space/fsf/watch
Hardware watchpoint 1: ptr[32768]
Hardware watchpoint 1: ptr[32768]
Hardware watchpoint 1: ptr[32768]

Old value = <unknown>
New value = 0 '\0'
main () at watch.c:10
10        ptr[32768] = 1;
(gdb) c
Continuing.
Hardware watchpoint 1: ptr[32768]

Old value = 0 '\0'
New value = 1 '\001'
main () at watch.c:11
11      }
(gdb)
Continuing.

Program exited with code 020.

The first <unknown> -> 0 transition is the initialization of "ptr".
Suddenly we're not dereferencing NULL any more, we're looking at some
newly allocated storage.  A hardware watchpoint on the pointer detects
this automatically.  The 0 -> 1 transition then is obvious.

This is much nicer :-)  Any opinions on this change?  Also, does it
deserve a NEWS entry and should it go in 6.7?

-- 
Daniel Jacobowitz
CodeSourcery

2007-08-21  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (breakpoint.o): Update.
	* breakpoint.c (insert_bp_location): Handle exceptions while
	evaluating and fetching the watchpoint expression.  Always watch
	the outermost value even if it is lazy.
	(insert_breakpoints, remove_breakpoint, bpstat_stop_status)
	(watch_command_1, can_use_hardware_watchpoint, breakpoint_re_set_one)
	(do_enable_breakpoint): Likewise.
	(watchpoint_check): Likewise.  Report changes when a value becomes
	readable or unreadable.
	(watchpoint_value_print): New.
	(print_it_typical): Use it.  Do not free or clear old_val.  Print
	watchpoints even if old_val == NULL.

2007-08-21  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.base/watchpoint.c (global_ptr, func4): New.
	(main): Call func4.
	* gdb.base/watchpoint.exp: Call test_inaccessible_watchpoint.
	(test_inaccessible_watchpoint): New.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.928
diff -u -p -r1.928 Makefile.in
--- Makefile.in	10 Aug 2007 17:52:09 -0000	1.928
+++ Makefile.in	21 Aug 2007 12:45:03 -0000
@@ -1853,7 +1853,7 @@ breakpoint.o: breakpoint.c $(defs_h) $(s
 	$(objfiles_h) $(source_h) $(linespec_h) $(completer_h) $(gdb_h) \
 	$(ui_out_h) $(cli_script_h) $(gdb_assert_h) $(block_h) $(solib_h) \
 	$(solist_h) $(observer_h) $(exceptions_h) $(gdb_events_h) \
-	$(mi_common_h) $(memattr_h) $(ada_lang_h) $(top_h)
+	$(mi_common_h) $(memattr_h) $(ada_lang_h) $(top_h) $(wrapper_h)
 bsd-kvm.o: bsd-kvm.c $(defs_h) $(cli_cmds_h) $(command_h) $(frame_h) \
 	$(regcache_h) $(target_h) $(value_h) $(gdbcore_h) $(gdb_assert_h) \
 	$(readline_h) $(bsd_kvm_h)
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.261
diff -u -p -r1.261 breakpoint.c
--- breakpoint.c	17 Aug 2007 17:06:02 -0000	1.261
+++ breakpoint.c	21 Aug 2007 13:18:28 -0000
@@ -56,6 +56,7 @@
 #include "memattr.h"
 #include "ada-lang.h"
 #include "top.h"
+#include "wrapper.h"
 
 #include "gdb-events.h"
 #include "mi/mi-common.h"
@@ -1025,8 +1026,6 @@ Note: automatically using hardware break
 	 convert this.  */
 
       int within_current_scope;
-      struct value *mark = value_mark ();
-      struct value *v;
       struct frame_id saved_frame_id;
 
       /* Save the current frame's ID so we can restore it after
@@ -1050,6 +1049,9 @@ Note: automatically using hardware break
 
       if (within_current_scope)
 	{
+	  struct value *mark = value_mark ();
+	  struct value *v = NULL;
+
 	  free_valchain (bpt);
 
 	  /* Evaluate the expression and cut the chain of values
@@ -1058,9 +1060,12 @@ Note: automatically using hardware break
 	     Make sure the value returned isn't lazy; we use
 	     laziness to determine what memory GDB actually needed
 	     in order to compute the value of the expression.  */
-	  v = evaluate_expression (bpt->owner->exp);
-	  value_contents (v);
-	  value_release_to_mark (mark);
+	  gdb_evaluate_expression (bpt->owner->exp, &v);
+	  if (v != NULL)
+	    {
+	      gdb_value_fetch_lazy (v);
+	      value_release_to_mark (mark);
+	    }
 
 	  bpt->owner->val_chain = v;
 	  bpt->inserted = 1;
@@ -1072,7 +1077,7 @@ Note: automatically using hardware break
 		 its contents to evaluate the expression, then we
 		 must watch it.  */
 	      if (VALUE_LVAL (v) == lval_memory
-		  && ! value_lazy (v))
+		  && (v == bpt->owner->val_chain || ! value_lazy (v)))
 		{
 		  struct type *vtype = check_typedef (value_type (v));
 
@@ -1255,11 +1260,13 @@ insert_breakpoints (void)
       if ((b->loc_type == bp_loc_hardware_watchpoint
 	   || b->owner->type == bp_watchpoint) && !b->owner->val)
 	{
-	  struct value *val;
-	  val = evaluate_expression (b->owner->exp);
-	  release_value (val);
-	  if (value_lazy (val))
-	    value_fetch_lazy (val);
+	  struct value *val = NULL;
+	  gdb_evaluate_expression (b->owner->exp, &val);
+	  if (val != NULL)
+	    {
+	      gdb_value_fetch_lazy (val);
+	      release_value (val);
+	    }
 	  b->owner->val = val;
 	}
 
@@ -1577,7 +1584,7 @@ remove_breakpoint (struct bp_location *b
 	  /* For each memory reference remove the watchpoint
 	     at that address.  */
 	  if (VALUE_LVAL (v) == lval_memory
-	      && ! value_lazy (v))
+	      && (v == b->owner->val_chain || ! value_lazy (v)))
 	    {
 	      struct type *vtype = check_typedef (value_type (v));
 
@@ -2129,6 +2136,17 @@ top:
   do_cleanups (old_chain);
 }
 
+/* Print out the (old or new) value associated with a watchpoint.  */
+
+static void
+watchpoint_value_print (struct value *val, struct ui_file *stream)
+{
+  if (val == NULL || value_lazy (val))
+    fprintf_unfiltered (stream, _("<unknown>"));
+  else
+    value_print (val, stream, 0, Val_pretty_default);
+}
+
 /* This is the normal print function for a bpstat.  In the future,
    much of this logic could (should?) be moved to bpstat_stop_status,
    by having it set different print_it values.
@@ -2305,26 +2323,21 @@ print_it_typical (bpstat bs)
 
     case bp_watchpoint:
     case bp_hardware_watchpoint:
-      if (bs->old_val != NULL)
-	{
-	  annotate_watchpoint (bs->breakpoint_at->number);
-	  if (ui_out_is_mi_like_p (uiout))
-	    ui_out_field_string
-	      (uiout, "reason",
-	       async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER));
-	  mention (bs->breakpoint_at);
-	  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
-	  ui_out_text (uiout, "\nOld value = ");
-	  value_print (bs->old_val, stb->stream, 0, Val_pretty_default);
-	  ui_out_field_stream (uiout, "old", stb);
-	  ui_out_text (uiout, "\nNew value = ");
-	  value_print (bs->breakpoint_at->val, stb->stream, 0, Val_pretty_default);
-	  ui_out_field_stream (uiout, "new", stb);
-	  do_cleanups (ui_out_chain);
-	  ui_out_text (uiout, "\n");
-	  value_free (bs->old_val);
-	  bs->old_val = NULL;
-	}
+      annotate_watchpoint (bs->breakpoint_at->number);
+      if (ui_out_is_mi_like_p (uiout))
+	ui_out_field_string
+	  (uiout, "reason",
+	   async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER));
+      mention (bs->breakpoint_at);
+      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+      ui_out_text (uiout, "\nOld value = ");
+      watchpoint_value_print (bs->old_val, stb->stream);
+      ui_out_field_stream (uiout, "old", stb);
+      ui_out_text (uiout, "\nNew value = ");
+      watchpoint_value_print (bs->breakpoint_at->val, stb->stream);
+      ui_out_field_stream (uiout, "new", stb);
+      do_cleanups (ui_out_chain);
+      ui_out_text (uiout, "\n");
       /* More than one watchpoint may have been triggered.  */
       return PRINT_UNKNOWN;
       break;
@@ -2337,7 +2350,7 @@ print_it_typical (bpstat bs)
       mention (bs->breakpoint_at);
       ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
       ui_out_text (uiout, "\nValue = ");
-      value_print (bs->breakpoint_at->val, stb->stream, 0, Val_pretty_default);
+      watchpoint_value_print (bs->breakpoint_at->val, stb->stream);
       ui_out_field_stream (uiout, "value", stb);
       do_cleanups (ui_out_chain);
       ui_out_text (uiout, "\n");
@@ -2345,7 +2358,7 @@ print_it_typical (bpstat bs)
       break;
 
     case bp_access_watchpoint:
-      if (bs->old_val != NULL)     
+      if (bs->old_val != NULL)
 	{
 	  annotate_watchpoint (bs->breakpoint_at->number);
 	  if (ui_out_is_mi_like_p (uiout))
@@ -2357,8 +2370,6 @@ print_it_typical (bpstat bs)
 	  ui_out_text (uiout, "\nOld value = ");
 	  value_print (bs->old_val, stb->stream, 0, Val_pretty_default);
 	  ui_out_field_stream (uiout, "old", stb);
-	  value_free (bs->old_val);
-	  bs->old_val = NULL;
 	  ui_out_text (uiout, "\nNew value = ");
 	}
       else 
@@ -2371,7 +2382,7 @@ print_it_typical (bpstat bs)
 	  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
 	  ui_out_text (uiout, "\nValue = ");
 	}
-      value_print (bs->breakpoint_at->val, stb->stream, 0,Val_pretty_default);
+      watchpoint_value_print (bs->breakpoint_at->val, stb->stream);
       ui_out_field_stream (uiout, "new", stb);
       do_cleanups (ui_out_chain);
       ui_out_text (uiout, "\n");
@@ -2590,11 +2601,23 @@ watchpoint_check (void *p)
          we might be in the middle of evaluating a function call.  */
 
       struct value *mark = value_mark ();
-      struct value *new_val = evaluate_expression (bs->breakpoint_at->exp);
-      if (!value_equal (b->val, new_val))
+      struct value *new_val = NULL;
+      int old_valid, new_valid;
+
+      gdb_evaluate_expression (bs->breakpoint_at->exp, &new_val);
+      if (new_val != NULL)
+	gdb_value_fetch_lazy (new_val);
+
+      old_valid = (b->val != NULL && ! value_lazy (b->val));
+      new_valid = (new_val != NULL && ! value_lazy (new_val));
+      if (old_valid != new_valid
+	  || (old_valid && !value_equal (b->val, new_val)))
 	{
-	  release_value (new_val);
-	  value_free_to_mark (mark);
+	  if (new_val != NULL)
+	    {
+	      release_value (new_val);
+	      value_free_to_mark (mark);
+	    }
 	  bs->old_val = b->val;
 	  b->val = new_val;
 	  /* We will stop here */
@@ -2824,7 +2847,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	for (v = b->val_chain; v; v = value_next (v))
 	  {
 	    if (VALUE_LVAL (v) == lval_memory
-		&& ! value_lazy (v))
+		&& (v == b->val_chain || ! value_lazy (v)))
 	      {
 		struct type *vtype = check_typedef (value_type (v));
 
@@ -5723,10 +5746,13 @@ watch_command_1 (char *arg, int accessfl
   exp_end = arg;
   exp_valid_block = innermost_block;
   mark = value_mark ();
-  val = evaluate_expression (exp);
-  release_value (val);
-  if (value_lazy (val))
-    value_fetch_lazy (val);
+  val = NULL;
+  gdb_evaluate_expression (exp, &val);
+  if (val != NULL)
+    {
+      gdb_value_fetch_lazy (val);
+      release_value (val);
+    }
 
   tok = arg;
   while (*tok == ' ' || *tok == '\t')
@@ -5894,7 +5920,7 @@ can_use_hardware_watchpoint (struct valu
     {
       if (VALUE_LVAL (v) == lval_memory)
 	{
-	  if (value_lazy (v))
+	  if (v != head && value_lazy (v))
 	    /* A lazy memory lvalue is one that GDB never needed to fetch;
 	       we either just used its address (e.g., `a' in `a.b') or
 	       we never needed it at all (e.g., `a' in `a,b').  */
@@ -7319,10 +7345,13 @@ breakpoint_re_set_one (void *bint)
              evaluate_expression.  */
 	  b->val = NULL;
 	}
-      b->val = evaluate_expression (b->exp);
-      release_value (b->val);
-      if (value_lazy (b->val) && breakpoint_enabled (b))
-	value_fetch_lazy (b->val);
+      gdb_evaluate_expression (b->exp, &b->val);
+      if (b->val)
+	{
+	  release_value (b->val);
+	  if (value_lazy (b->val) && breakpoint_enabled (b))
+	    gdb_value_fetch_lazy (b->val);
+	}
 
       if (b->cond_string != NULL)
 	{
@@ -7675,10 +7704,13 @@ is valid is not currently in scope.\n"),
 	  
 	  value_free (bpt->val);
 	  mark = value_mark ();
-	  bpt->val = evaluate_expression (bpt->exp);
-	  release_value (bpt->val);
-	  if (value_lazy (bpt->val))
-	    value_fetch_lazy (bpt->val);
+	  bpt->val = NULL;
+	  gdb_evaluate_expression (bpt->exp, &bpt->val);
+	  if (bpt->val)
+	    {
+	      release_value (bpt->val);
+	      gdb_value_fetch_lazy (bpt->val);
+	    }
 	  
 	  if (bpt->type == bp_hardware_watchpoint ||
 	      bpt->type == bp_read_watchpoint ||
Index: testsuite/gdb.base/watchpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/watchpoint.c,v
retrieving revision 1.2
diff -u -p -r1.2 watchpoint.c
--- testsuite/gdb.base/watchpoint.c	17 Mar 2003 19:51:58 -0000	1.2
+++ testsuite/gdb.base/watchpoint.c	21 Aug 2007 14:23:20 -0000
@@ -39,6 +39,8 @@ struct foo struct1, struct2, *ptr1, *ptr
 
 int doread = 0;
 
+char *global_ptr;
+
 void marker1 ()
 {
 }
@@ -110,6 +112,14 @@ func1 ()
   return 73;
 }
 
+void
+func4 ()
+{
+  buf[0] = 3;
+  global_ptr = buf;
+  buf[0] = 7;
+}
+
 int main ()
 {
 #ifdef usestubs
@@ -185,5 +195,7 @@ int main ()
 
   func3 ();
 
+  func4 ();
+
   return 0;
 }
Index: testsuite/gdb.base/watchpoint.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/watchpoint.exp,v
retrieving revision 1.14
diff -u -p -r1.14 watchpoint.exp
--- testsuite/gdb.base/watchpoint.exp	2 Mar 2007 22:11:15 -0000	1.14
+++ testsuite/gdb.base/watchpoint.exp	21 Aug 2007 14:23:20 -0000
@@ -646,6 +646,30 @@ proc test_watchpoint_and_breakpoint {} {
     }
 }
     
+proc test_inaccessible_watchpoint {} {
+    global gdb_prompt
+
+    # This is a test for watchpoints on currently inaccessible (but later
+    # valid) memory.
+
+    if [runto func4] then {
+	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
+	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test_multiple "next" "next over ptr init" {
+	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
+		# We can not test for <unknown> here because NULL may be readable.
+		# This test does rely on *NULL != 3.
+		pass "next over ptr init"
+	    }
+	}
+	gdb_test_multiple "next" "next over buffer set" {
+	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = 3 .*\r\nNew value = 7 .*\r\n.*$gdb_prompt $" {
+		pass "next over buffer set"
+	    }
+	}
+    }
+}
+    
 # Start with a fresh gdb.
 
 gdb_exit
@@ -798,6 +822,8 @@ if [initialize] then {
       }
     }
 
+    test_inaccessible_watchpoint
+
     # See above.
     if [istarget "mips-idt-*"] then {
 	gdb_exit


^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-03-03 14:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-08-21 14:25 [rfc] Allow watchpoints on inaccessible memory Daniel Jacobowitz
2007-08-21 16:33 ` Jim Blandy
2007-08-21 18:06   ` Daniel Jacobowitz
2007-08-21 22:49     ` Jim Blandy
2007-08-21 23:17       ` Daniel Jacobowitz
2007-08-21 19:04 ` Eli Zaretskii
2007-11-20  7:56 ` Vladimir Prus
2008-02-28 16:27 ` Daniel Jacobowitz
2008-02-29 10:11   ` Eli Zaretskii
2008-03-02  4:24     ` Daniel Jacobowitz
2008-03-02 21:09       ` Eli Zaretskii
2008-03-02 23:23         ` Daniel Jacobowitz
2008-03-03  4:27           ` Eli Zaretskii
2008-03-03 14:07             ` Daniel Jacobowitz
2008-02-29 17:54   ` Jim Blandy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox