From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25591 invoked by alias); 3 Jul 2008 01:27:16 -0000 Received: (qmail 25579 invoked by uid 22791); 3 Jul 2008 01:27:14 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 03 Jul 2008 01:26:57 +0000 Received: (qmail 23729 invoked from network); 3 Jul 2008 01:26:54 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 3 Jul 2008 01:26:54 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: functions that delete breakpoints should not insert bp locations (was: Fix execl.exp sporadic failures) Date: Thu, 03 Jul 2008 01:27:00 -0000 User-Agent: KMail/1.9.9 MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_erCbIAL5IsylB2k" Message-Id: <200807030226.54896.pedro@codesourcery.com> 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: 2008-07/txt/msg00026.txt.bz2 --Boundary-00=_erCbIAL5IsylB2k Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 3539 As I've said in my previous post, thread_db has a call to remove_thread_event_breakpoints after the inferior having execd, here: static ptid_t thread_db_wait (ptid_t ptid, struct target_waitstatus *ourstatus) { ptid = target_beneath->to_wait (ptid, ourstatus); (...) if (ourstatus->kind == TARGET_WAITKIND_EXECD) { remove_thread_event_breakpoints (); <<<<< unpush_target (&thread_db_ops); using_thread_db = 0; return ptid; } breakpoint.c:remove_thread_event_breakpoints is this: void remove_thread_event_breakpoints (void) { struct breakpoint *b, *temp; ALL_BREAKPOINTS_SAFE (b, temp) if (b->type == bp_thread_event) delete_breakpoint (b); } With the previous patch I just sent in place, which marks the breakpoints not inserted before reaching this point, delete_breakpoint (and its callees), will no longer try to remove the breakpoint locations from the inferior. The problem with always inserted mode, is that update_global_location_list, the workhorse that detects duplicate locations, and if locations should be deleted or inserted, finds that breakpoint locations tagged as not inserted but which are enabled, and so goes on and inserts them. But the addresses in which they are being inserted don't make any sense in the new exec image, and we again see errors like: /home/pedro/gdb/execl_hunt/build32/gdb/testsuite/gdb.threads/execl1: error while loading shared libraries: libm.so.6: failed to map segment from shared object: Cannot allocate memory IMO, having the breakpoints management functions whose job is to delete a breakpoint insert locations, is a side effect that is always undesirable. Here's another example I found while working on non-stop, which requires always-inserted mode, that is broken due to that same fact: void target_detach (char *args, int from_tty) { /* If we're in breakpoints-always-inserted mode, have to remove them before detaching. */ remove_breakpoints (); (current_target.to_detach) (args, from_tty); } static void thread_db_detach (char *args, int from_tty) { disable_thread_event_reporting (); target_beneath->to_detach (args, from_tty); /* Should this be done by detach_command? */ target_mourn_inferior (); } static void disable_thread_event_reporting (void) { td_thr_events_t events; /* Set the process wide mask saying we aren't interested in any events anymore. */ td_event_emptyset (&events); td_ta_set_event_p (thread_agent, &events); /* Delete thread event breakpoints, if any. */ remove_thread_event_breakpoints (); <<<<<< td_create_bp_addr = 0; td_death_bp_addr = 0; } Notice that first, all breakpoints are removed from the inferior in target_detach, but if the inferior has thread_db loaded, the remove_thread_event_breakpoints call, which calls delete_breakpoint, will end up reinserting the locations into the inferior, with the effect that the inferior crashes with hitting a left over breakpoint right after detaching. IMO, the best solution to these problems is making sure that update_global_location_list does not insert breakpoints locations if being called from a function that is deleting (a) breakpoint(s). This also gets rid of the hack of temporarilly disabling always-inserted mode in update_breakpoints_after_exec, which was there due to this exact fact. Tested on x86_64-unknown-linux-gnu {,-m32}, with and without always-inserted mode. No regressions, and execl.exp now passes cleanly in all combinations, always. OK? -- Pedro Alves --Boundary-00=_erCbIAL5IsylB2k Content-Type: text/x-diff; charset="utf-8"; name="breakpoint_delete_no_insert.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="breakpoint_delete_no_insert.diff" Content-length: 10309 2008-07-03 Pedro Alves * breakpoint.c (update_global_location_list): Add boolean "inserting" argument. Only insert locations if caller told it too. (update_global_location_list_nothrow): Add boolean "inserting" argument. Pass it to update_global_location_list. (insert_breakpoints, create_longjmp_breakpoint) (create_overlay_event_breakpoint, enable_overlay_breakpoints) (create_thread_event_breakpoint, create_solib_event_breakpoint) (create_fork_vfork_event_catchpoint, create_exec_event_catchpoint) (enable_watchpoints_after_interactive_call_stop) (set_momentary_breakpoint, create_breakpoints) (break_command_really, watch_command_1) (create_ada_exception_breakpoint, update_breakpoint_locations) (do_enable_breakpoint, enable_command): Pass true to update_global_location_list. (bpstat_stop_status, disable_overlay_breakpoints) (disable_watchpoints_before_interactive_call_start) (delete_breakpoint, disable_breakpoint, disable_command): Pass false to update_global_location_list. (update_breakpoints_after_exec): Don't temporarily disable always-inserted mode. --- gdb/breakpoint.c | 71 +++++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 38 deletions(-) Index: src/gdb/breakpoint.c =================================================================== --- src.orig/gdb/breakpoint.c 2008-07-03 02:06:48.000000000 +0100 +++ src/gdb/breakpoint.c 2008-07-03 02:24:25.000000000 +0100 @@ -191,9 +191,9 @@ static void free_bp_location (struct bp_ static struct bp_location * allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type); -static void update_global_location_list (void); +static void update_global_location_list (int inserting); -static void update_global_location_list_nothrow (void); +static void update_global_location_list_nothrow (int inserting); static int is_hardware_watchpoint (struct breakpoint *bpt); @@ -1264,7 +1264,7 @@ insert_breakpoints (void) if (is_hardware_watchpoint (bpt)) update_watchpoint (bpt, 0 /* don't reparse. */); - update_global_location_list (); + update_global_location_list (1); if (!always_inserted_mode && target_has_execution) /* update_global_location_list does not insert breakpoints @@ -1441,7 +1441,6 @@ update_breakpoints_after_exec (void) { struct breakpoint *b; struct breakpoint *temp; - struct cleanup *cleanup; /* There used to be a call to mark_breakpoints_out here with the following comment: @@ -1456,13 +1455,6 @@ update_breakpoints_after_exec (void) reaching here. The call has since moved closer to where the each target detects an exec. */ - - /* The binary we used to debug is now gone, and we're updating - breakpoints for the new binary. Until we're done, we should not - try to insert breakpoints. */ - cleanup = make_cleanup_restore_integer (&always_inserted_mode); - always_inserted_mode = 0; - ALL_BREAKPOINTS_SAFE (b, temp) { /* Solib breakpoints must be explicitly reset after an exec(). */ @@ -1552,7 +1544,6 @@ update_breakpoints_after_exec (void) } /* FIXME what about longjmp breakpoints? Re-create them here? */ create_overlay_event_breakpoint ("_ovly_debug_event"); - do_cleanups (cleanup); } int @@ -3069,7 +3060,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, p if (b->disposition == disp_disable) { b->enable_state = bp_disabled; - update_global_location_list (); + update_global_location_list (0); } if (b->silent) bs->print = 0; @@ -4481,7 +4472,7 @@ create_longjmp_breakpoint (char *func_na if ((m = lookup_minimal_symbol_text (func_name, NULL)) == NULL) return; set_momentary_breakpoint_at_pc (SYMBOL_VALUE_ADDRESS (m), bp_longjmp); - update_global_location_list (); + update_global_location_list (1); } /* Call this routine when stepping and nexting to enable a breakpoint @@ -4539,7 +4530,7 @@ create_overlay_event_breakpoint (char *f b->enable_state = bp_disabled; overlay_events_enabled = 0; } - update_global_location_list (); + update_global_location_list (1); } void @@ -4551,7 +4542,7 @@ enable_overlay_breakpoints (void) if (b->type == bp_overlay_event) { b->enable_state = bp_enabled; - update_global_location_list (); + update_global_location_list (1); overlay_events_enabled = 1; } } @@ -4565,7 +4556,7 @@ disable_overlay_breakpoints (void) if (b->type == bp_overlay_event) { b->enable_state = bp_disabled; - update_global_location_list (); + update_global_location_list (0); overlay_events_enabled = 0; } } @@ -4581,7 +4572,7 @@ create_thread_event_breakpoint (CORE_ADD /* addr_string has to be used or breakpoint_re_set will delete me. */ b->addr_string = xstrprintf ("*0x%s", paddr (b->loc->address)); - update_global_location_list_nothrow (); + update_global_location_list_nothrow (1); return b; } @@ -4627,7 +4618,7 @@ create_solib_event_breakpoint (CORE_ADDR struct breakpoint *b; b = create_internal_breakpoint (address, bp_shlib_event); - update_global_location_list_nothrow (); + update_global_location_list_nothrow (1); return b; } @@ -4725,7 +4716,7 @@ create_fork_vfork_event_catchpoint (int b->enable_state = bp_enabled; b->disposition = tempflag ? disp_del : disp_donttouch; b->forked_inferior_pid = 0; - update_global_location_list (); + update_global_location_list (1); mention (b); @@ -4764,7 +4755,7 @@ create_exec_event_catchpoint (int tempfl b->addr_string = NULL; b->enable_state = bp_enabled; b->disposition = tempflag ? disp_del : disp_donttouch; - update_global_location_list (); + update_global_location_list (1); mention (b); } @@ -4820,7 +4811,7 @@ disable_watchpoints_before_interactive_c && breakpoint_enabled (b)) { b->enable_state = bp_call_disabled; - update_global_location_list (); + update_global_location_list (0); } } } @@ -4839,7 +4830,7 @@ enable_watchpoints_after_interactive_cal && (b->enable_state == bp_call_disabled)) { b->enable_state = bp_enabled; - update_global_location_list (); + update_global_location_list (1); } } } @@ -4865,7 +4856,7 @@ set_momentary_breakpoint (struct symtab_ if (in_thread_list (inferior_ptid)) b->thread = pid_to_thread_id (inferior_ptid); - update_global_location_list_nothrow (); + update_global_location_list_nothrow (1); return b; } @@ -5287,7 +5278,7 @@ create_breakpoints (struct symtabs_and_l thread, ignore_count, ops, from_tty); } - update_global_location_list (); + update_global_location_list (1); } /* Parse ARG which is assumed to be a SAL specification possibly @@ -5608,7 +5599,7 @@ break_command_really (char *arg, char *c b->condition_not_parsed = 1; b->ops = ops; - update_global_location_list (); + update_global_location_list (1); mention (b); } @@ -6037,7 +6028,7 @@ watch_command_1 (char *arg, int accessfl value_free_to_mark (mark); mention (b); - update_global_location_list (); + update_global_location_list (1); } /* Return count of locations need to be watched and can be handled @@ -6675,7 +6666,7 @@ create_ada_exception_breakpoint (struct b->ops = ops; mention (b); - update_global_location_list (); + update_global_location_list (1); } /* Implement the "catch exception" command. */ @@ -7000,7 +6991,7 @@ breakpoint_auto_delete (bpstat bs) } static void -update_global_location_list (void) +update_global_location_list (int inserting) { struct breakpoint *b; struct bp_location **next = &bp_location_chain; @@ -7132,7 +7123,11 @@ update_global_location_list (void) check_duplicates (b); } - if (always_inserted_mode && target_has_execution) + /* If we get here due to deleting a breakpoint, don't try to insert + locations. This helps cases like: target_detach deleting a + breakpoint before detaching causing all other breakpoints to be + inserted. */ + if (always_inserted_mode && inserting && target_has_execution) insert_breakpoint_locations (); } @@ -7152,11 +7147,11 @@ breakpoint_retire_moribund (void) } static void -update_global_location_list_nothrow (void) +update_global_location_list_nothrow (int inserting) { struct gdb_exception e; TRY_CATCH (e, RETURN_MASK_ERROR) - update_global_location_list (); + update_global_location_list (inserting); } /* Delete a breakpoint and clean up all traces of it in the data @@ -7246,7 +7241,7 @@ delete_breakpoint (struct breakpoint *bp looks at location's owner. It might be better design to have location completely self-contained, but it's not the case now. */ - update_global_location_list (); + update_global_location_list (0); /* On the chance that someone will soon try again to delete this same @@ -7456,7 +7451,7 @@ update_breakpoint_locations (struct brea } } - update_global_location_list (); + update_global_location_list (1); } @@ -7842,7 +7837,7 @@ disable_breakpoint (struct breakpoint *b bpt->enable_state = bp_disabled; - update_global_location_list (); + update_global_location_list (0); if (deprecated_modify_breakpoint_hook) deprecated_modify_breakpoint_hook (bpt); @@ -7881,7 +7876,7 @@ disable_command (char *args, int from_tt struct bp_location *loc = find_location_by_number (args); if (loc) loc->enabled = 0; - update_global_location_list (); + update_global_location_list (0); } else map_breakpoint_numbers (args, disable_breakpoint); @@ -7966,7 +7961,7 @@ have been allocated for other watchpoint if (bpt->enable_state != bp_permanent) bpt->enable_state = bp_enabled; bpt->disposition = disposition; - update_global_location_list (); + update_global_location_list (1); breakpoints_changed (); if (deprecated_modify_breakpoint_hook) @@ -8017,7 +8012,7 @@ enable_command (char *args, int from_tty struct bp_location *loc = find_location_by_number (args); if (loc) loc->enabled = 1; - update_global_location_list (); + update_global_location_list (1); } else map_breakpoint_numbers (args, enable_breakpoint); --Boundary-00=_erCbIAL5IsylB2k--