From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10660 invoked by alias); 18 Aug 2004 13:56:26 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 10643 invoked from network); 18 Aug 2004 13:56:22 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sourceware.org with SMTP; 18 Aug 2004 13:56:22 -0000 Received: from drow by nevyn.them.org with local (Exim 4.34 #1 (Debian)) id 1BxQvN-0000Jz-VQ; Wed, 18 Aug 2004 09:56:22 -0400 Date: Wed, 18 Aug 2004 13:56:00 -0000 From: Daniel Jacobowitz To: Jeff Johnston Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: Fix for pending breakpoints in manually loaded/unloaded shlibs Message-ID: <20040818135621.GA26257@nevyn.them.org> Mail-Followup-To: Jeff Johnston , gdb-patches@sources.redhat.com References: <41191D71.60204@redhat.com> <20040811171203.GA4152@nevyn.them.org> <411A7D97.50104@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <411A7D97.50104@redhat.com> User-Agent: Mutt/1.5.5.1+cvs20040105i X-SW-Source: 2004-08/txt/msg00555.txt.bz2 On Wed, Aug 11, 2004 at 04:12:07PM -0400, Jeff Johnston wrote: > Ok? Sorry to keep picking nits; while we discuss the issue of the new observer I went over the patch again for minor problems. > +/* Disable any breakpoints that are in in an unloaded shared library. Only > + apply to enabled breakpoints, disabled ones can just stay disabled. */ > + > +void > +disable_breakpoints_in_unloaded_shlib (struct so_list *solib) > +{ > + struct breakpoint *b; > + int disabled_shlib_breaks = 0; > + > + /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */ Two spaces after a period, please. > + ALL_BREAKPOINTS (b) > + { > +#if defined (PC_SOLIB) I think someone pointed out that you've #ifdef'd out the entire body of this loop. Might as well include the whole loop. The #ifdef is nasty, but that's a preexisting problem. > + if ((b->loc->loc_type == bp_loc_hardware_breakpoint > + || b->loc->loc_type == bp_loc_software_breakpoint) > + && breakpoint_enabled (b) > + && !b->loc->duplicate) > + { > + char *so_name = PC_SOLIB (b->loc->address); While I'm ranting about preexisting problems, it would be nice if PC_SOLIB returned the solib, instead of just its name... but enh. > + if (so_name > + && !strcmp (so_name, solib->so_name)) > + { > + b->enable_state = bp_shlib_disabled; > + /* At this point, we cannot rely on remove_breakpoint > + succeeding so we must mark the breakpoint as not inserted > + to prevent future errors occurring in remove_breakpoints. */ > + b->loc->inserted = 0; > + if (!disabled_shlib_breaks) > + { > + target_terminal_ours_for_output (); > + warning ("Temporarily disabling unloaded shared library breakpoints:"); > + } > + disabled_shlib_breaks = 1; > + warning ("breakpoint #%d ", b->number); I think you're missing a space after the colon, in the first warning. Also, this use of multiple warning() statements is neither i18n friendly nor MI/GUI friendly - you may get a separate dialog box for each. I believe other places do this with sprintf; still not 100% i18n friendly, but avoids the MI/GUI problems. I can't find an example offhand. -- Daniel Jacobowitz