From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22515 invoked by alias); 21 Aug 2007 14:25:13 -0000 Received: (qmail 22185 invoked by uid 22791); 21 Aug 2007 14:25:08 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 21 Aug 2007 14:25:03 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 80032980C1 for ; Tue, 21 Aug 2007 14:25:03 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 2A45D980C0 for ; Tue, 21 Aug 2007 14:25:03 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.67) (envelope-from ) id 1INUfE-0007tb-IM for gdb-patches@sourceware.org; Tue, 21 Aug 2007 10:25:00 -0400 Date: Tue, 21 Aug 2007 14:25:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sourceware.org Subject: [rfc] Allow watchpoints on inaccessible memory Message-ID: <20070821142500.GA28295@caradoc.them.org> Mail-Followup-To: gdb-patches@sourceware.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.15 (2007-04-09) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-08/txt/msg00417.txt.bz2 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 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 = 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 -> 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 * 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 * 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, _("")); + 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 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