From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10209 invoked by alias); 10 Jun 2010 20:39:36 -0000 Received: (qmail 10196 invoked by uid 22791); 10 Jun 2010 20:39:34 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 10 Jun 2010 20:39:26 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5AKdI11016212 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 10 Jun 2010 16:39:18 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5AKdFFp007459 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 10 Jun 2010 16:39:17 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o5AKdFeL011720; Thu, 10 Jun 2010 22:39:15 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o5AKdE3W011716; Thu, 10 Jun 2010 22:39:14 +0200 Date: Thu, 10 Jun 2010 20:39:00 -0000 From: Jan Kratochvil To: Frederic Riss Cc: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff] Message-ID: <20100610203913.GA11204@host0.dyn.jankratochvil.net> References: <20100503200201.GB30386@host0.dyn.jankratochvil.net> <201005232240.17414.pedro@codesourcery.com> <20100604191855.GA29142@host0.dyn.jankratochvil.net> <201006071250.31842.pedro@codesourcery.com> <20100607134011.GA10971@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-12-10) 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/msg00252.txt.bz2 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. 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"