From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1676 invoked by alias); 11 Jun 2010 10:50:18 -0000 Received: (qmail 1193 invoked by uid 22791); 11 Jun 2010 10:50:14 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 11 Jun 2010 10:50:08 +0000 Received: (qmail 25839 invoked from network); 11 Jun 2010 10:50:06 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 11 Jun 2010 10:50:06 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff] Date: Fri, 11 Jun 2010 10:50:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.32-22-generic; KDE/4.4.2; x86_64; ; ) Cc: Frederic Riss , gdb-patches@sourceware.org References: <20100503200201.GB30386@host0.dyn.jankratochvil.net> <20100610203913.GA11204@host0.dyn.jankratochvil.net> In-Reply-To: <20100610203913.GA11204@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201006111150.04054.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: 2010-06/txt/msg00257.txt.bz2 On Thursday 10 June 2010 21:39:13, Jan Kratochvil wrote: > On Thu, 10 Jun 2010 16:08:46 +0200, Frederic Riss wrote: > > Seems that this commit is partially broken. In non-stop mode, after > > deleteing a step breakpoint, GDB call print_it_typical () on a bpstat > > that references that moribund breakpoint. > > :-( I was wrong and not precise enough checking for my: > > On Fri, 04 Jun 2010 21:18:55 +0200, Jan Kratochvil wrote: > # Other bs->breakpoint_at->owner dereferencing code either checks it is NULL > # or such code cannot meet bpstat referencing moribund bp_location. > > > Is this patch OK? I understand the former Pedro A.'s suggested way would not > have this regression. Okay. > > No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu. > > > Sorry, > Jan > > > gdb/ > 2010-06-10 Jan Kratochvil > > * breakpoint.c (breakpoint_restore_shadows): New OWNER comment. > (should_be_inserted): Return zero also on NULL OWNER. > (breakpoint_program_space_exit): New OWNER comment. > (insert_breakpoint_locations): Extend comment for OWNER. > (remove_breakpoint_1, remove_breakpoint): Assert on OWNER. > (breakpoint_init_inferior, breakpoint_here_p, breakpoint_thread_match): > New OWNER comment. > (print_it_typical): Return PRINT_UNKNOWN on NULL OWNER. > (watchpoint_check): New assert on BREAKPOINT_AT and OWNER. > (bpstat_check_location): New assert on OWNER. > (bpstat_check_watchpoint, bpstat_check_breakpoint_conditions): Move BL > and B initializations to the code block. New assert on them. > (print_one_breakpoint_location): New OWNER comment. > (watchpoint_locations_match): Assert on OWNER. > (breakpoint_locations_match): Move HW_POINT1 and HW_POINT2 > initializations to the code block. New assert on OWNER. > (set_breakpoint_location_function): New assert on OWNER. > (disable_breakpoints_in_shlibs, disable_breakpoints_in_unloaded_shlib) > (bp_location_compare, update_global_location_list) > (update_global_location_list): New OWNER comment. > > gdb/testsuite/ > 2010-06-10 Jan Kratochvil > > * gdb.base/moribund-step.exp: New. > > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1112,6 +1112,7 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len) > int bp_size = 0; > int bptoffset = 0; > > + /* bp_location array has B->OWNER always non-NULL. */ > if (b->owner->type == bp_none) > warning (_("reading through apparently deleted breakpoint #%d?"), > b->owner->number); > @@ -1571,7 +1572,7 @@ in which its expression is valid.\n"), > static int > should_be_inserted (struct bp_location *bpt) > { > - if (!breakpoint_enabled (bpt->owner)) > + if (bpt->owner == NULL || !breakpoint_enabled (bpt->owner)) > return 0; > > if (bpt->owner->disposition == disp_del_at_next_stop) > @@ -1874,6 +1875,7 @@ breakpoint_program_space_exit (struct program_space *pspace) > > if (loc->pspace == pspace) > { > + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */ > if (loc->owner->loc == loc) > loc->owner->loc = loc->next; > else > @@ -1943,7 +1945,8 @@ insert_breakpoint_locations (void) > continue; > > /* There is no point inserting thread-specific breakpoints if the > - thread no longer exists. */ > + thread no longer exists. ALL_BP_LOCATIONS bp_location has B->OWNER > + always non-NULL. */ > if (b->owner->thread != -1 > && !valid_thread_id (b->owner->thread)) > continue; > @@ -2394,6 +2397,9 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is) > { > int val; > > + /* B is never in moribund_locations by our callers. */ > + gdb_assert (b->owner != NULL); > + > if (b->owner->enable_state == bp_permanent) > /* Permanent breakpoints cannot be inserted or removed. */ > return 0; > @@ -2509,6 +2515,9 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is) > int ret; > struct cleanup *old_chain; > > + /* B is never in moribund_locations by our callers. */ > + gdb_assert (b->owner != NULL); > + > if (b->owner->enable_state == bp_permanent) > /* Permanent breakpoints cannot be inserted or removed. */ > return 0; > @@ -2566,6 +2575,7 @@ breakpoint_init_inferior (enum inf_context context) > > ALL_BP_LOCATIONS (bpt, bptp_tmp) > { > + /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL. */ > if (bpt->pspace == pspace > && bpt->owner->enable_state != bp_permanent) > bpt->inserted = 0; > @@ -2663,6 +2673,7 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc) > && bpt->loc_type != bp_loc_hardware_breakpoint) > continue; > > + /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL. */ > if ((breakpoint_enabled (bpt->owner) > || bpt->owner->enable_state == bp_permanent) > && breakpoint_address_match (bpt->pspace->aspace, bpt->address, > @@ -2827,6 +2838,7 @@ breakpoint_thread_match (struct address_space *aspace, CORE_ADDR pc, > && bpt->loc_type != bp_loc_hardware_breakpoint) > continue; > > + /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL. */ > if (!breakpoint_enabled (bpt->owner) > && bpt->owner->enable_state != bp_permanent) > continue; > @@ -3193,6 +3205,11 @@ print_it_typical (bpstat bs) > if (bs->breakpoint_at == NULL) > return PRINT_UNKNOWN; > bl = bs->breakpoint_at; > + > + /* bl->owner can be NULL if it was a momentary breakpoint > + which has since been placed into moribund_locations. */ > + if (bl->owner == NULL) > + return PRINT_UNKNOWN; > b = bl->owner; > > stb = ui_out_stream_new (uiout); > @@ -3563,6 +3580,9 @@ watchpoint_check (void *p) > struct frame_info *fr; > int within_current_scope; > > + /* BS is built for existing struct breakpoint. */ > + gdb_assert (bs->breakpoint_at != NULL); > + gdb_assert (bs->breakpoint_at->owner != NULL); > b = bs->breakpoint_at->owner; > > /* If this is a local watchpoint, we only want to check if the > @@ -3691,6 +3711,9 @@ bpstat_check_location (const struct bp_location *bl, > { > struct breakpoint *b = bl->owner; > > + /* BL is from existing struct breakpoint. */ > + gdb_assert (b != NULL); > + > /* By definition, the inferior does not report stops at > tracepoints. */ > if (is_tracepoint (b)) > @@ -3746,8 +3769,14 @@ bpstat_check_location (const struct bp_location *bl, > static void > bpstat_check_watchpoint (bpstat bs) > { > - const struct bp_location *bl = bs->breakpoint_at; > - struct breakpoint *b = bl->owner; > + const struct bp_location *bl; > + struct breakpoint *b; > + > + /* BS is built for existing struct breakpoint. */ > + bl = bs->breakpoint_at; > + gdb_assert (bl != NULL); > + b = bl->owner; > + gdb_assert (b != NULL); > > if (is_watchpoint (b)) > { > @@ -3899,8 +3928,14 @@ static void > bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) > { > int thread_id = pid_to_thread_id (ptid); > - const struct bp_location *bl = bs->breakpoint_at; > - struct breakpoint *b = bl->owner; > + const struct bp_location *bl; > + struct breakpoint *b; > + > + /* BS is built for existing struct breakpoint. */ > + bl = bs->breakpoint_at; > + gdb_assert (bl != NULL); > + b = bl->owner; > + gdb_assert (b != NULL); > > if (frame_id_p (b->frame_id) > && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ()))) > @@ -4678,6 +4713,8 @@ print_one_breakpoint_location (struct breakpoint *b, > || (!gdbarch_has_global_breakpoints (target_gdbarch) > && (number_of_program_spaces () > 1 > || number_of_inferiors () > 1) > + /* LOC is for existing B, it cannot be in moribund_locations and > + thus having NULL OWNER. */ > && loc->owner->type != bp_catchpoint))) > { > struct inferior *inf; > @@ -5214,6 +5250,10 @@ breakpoint_address_is_meaningful (struct breakpoint *bpt) > static int > watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2) > { > + /* Both of them must not be in moribund_locations. */ > + gdb_assert (loc1->owner != NULL); > + gdb_assert (loc2->owner != NULL); > + > /* Note that this checks the owner's type, not the location's. In > case the target does not support read watchpoints, but does > support access watchpoints, we'll have bp_read_watchpoint > @@ -5247,8 +5287,14 @@ breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1, > static int > breakpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2) > { > - int hw_point1 = is_hardware_watchpoint (loc1->owner); > - int hw_point2 = is_hardware_watchpoint (loc2->owner); > + int hw_point1, hw_point2; > + > + /* Both of them must not be in moribund_locations. */ > + gdb_assert (loc1->owner != NULL); > + gdb_assert (loc2->owner != NULL); > + > + hw_point1 = is_hardware_watchpoint (loc1->owner); > + hw_point2 = is_hardware_watchpoint (loc2->owner); > > if (hw_point1 != hw_point2) > return 0; > @@ -5445,6 +5491,8 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch, > static void > set_breakpoint_location_function (struct bp_location *loc) > { > + gdb_assert (loc->owner != NULL); > + > if (loc->owner->type == bp_breakpoint > || loc->owner->type == bp_hardware_breakpoint > || is_tracepoint (loc->owner)) > @@ -5727,7 +5775,9 @@ disable_breakpoints_in_shlibs (void) > > ALL_BP_LOCATIONS (loc, locp_tmp) > { > + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */ > struct breakpoint *b = loc->owner; > + > /* We apply the check to all breakpoints, including disabled > for those with loc->duplicate set. This is so that when breakpoint > becomes enabled, or the duplicate is removed, gdb will try to insert > @@ -5770,6 +5820,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib) > > ALL_BP_LOCATIONS (loc, locp_tmp) > { > + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */ > struct breakpoint *b = loc->owner; > > if ((loc->loc_type == bp_loc_hardware_breakpoint > @@ -8848,6 +8899,7 @@ bp_location_compare (const void *ap, const void *bp) > { > struct bp_location *a = *(void **) ap; > struct bp_location *b = *(void **) bp; > + /* A and B come from existing breakpoints having non-NULL OWNER. */ > int a_perm = a->owner->enable_state == bp_permanent; > int b_perm = b->owner->enable_state == bp_permanent; > > @@ -9025,6 +9077,7 @@ update_global_location_list (int should_insert) > See if there's another location at the same address, in which > case we don't need to remove this one from the target. */ > > + /* OLD_LOC comes from existing struct breakpoint. */ > if (breakpoint_address_is_meaningful (old_loc->owner)) > { > for (loc2p = locp; > @@ -9157,6 +9210,7 @@ update_global_location_list (int should_insert) > rwp_loc_first = NULL; > ALL_BP_LOCATIONS (loc, locp) > { > + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */ > struct breakpoint *b = loc->owner; > struct bp_location **loc_first_p; > > --- /dev/null > +++ b/gdb/testsuite/gdb.base/moribund-step.exp > @@ -0,0 +1,28 @@ > +# Copyright (C) 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 . > + > +set testfile moribund-step > +if { [prepare_for_testing ${testfile}.exp ${testfile} start.c] } { > + return -1 > +} > + > +gdb_test_no_output "set non-stop on" > + > +if ![runto_main] { > + return > +} > + > +# GDB could crash on printing breakpoint hit on a moribund bp_location. > +gdb_test "step" > -- Pedro Alves