From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4510 invoked by alias); 18 Aug 2004 19:22:29 -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 4498 invoked from network); 18 Aug 2004 19:22:28 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 18 Aug 2004 19:22:28 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.10/8.12.10) with ESMTP id i7IJMNe1027170 for ; Wed, 18 Aug 2004 15:22:23 -0400 Received: from pobox.toronto.redhat.com (pobox.toronto.redhat.com [172.16.14.4]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i7IJMNa26045; Wed, 18 Aug 2004 15:22:23 -0400 Received: from touchme.toronto.redhat.com (IDENT:postfix@touchme.toronto.redhat.com [172.16.14.9]) by pobox.toronto.redhat.com (8.12.8/8.12.8) with ESMTP id i7IJMMse018934; Wed, 18 Aug 2004 15:22:22 -0400 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id C4E6480001E; Wed, 18 Aug 2004 15:22:22 -0400 (EDT) Message-ID: <4123AC6E.8000300@redhat.com> Date: Wed, 18 Aug 2004 19:22:00 -0000 From: Jeff Johnston User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 MIME-Version: 1.0 To: Daniel Jacobowitz Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: Fix for pending breakpoints in manually loaded/unloaded shlibs References: <41191D71.60204@redhat.com> <20040811171203.GA4152@nevyn.them.org> <411A7D97.50104@redhat.com> <20040818135621.GA26257@nevyn.them.org> In-Reply-To: <20040818135621.GA26257@nevyn.them.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2004-08/txt/msg00564.txt.bz2 Daniel Jacobowitz wrote: > 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. > What you do want to see so I don't waste my time on this. As you already know, this routine was copied from the routine which disables shared library breakpoints in breakpoint.c. Is it sufficient to just issue the warning that I am temporarily disabling unloaded shared library breakpoints and not spell out each breakpoint in turn? I can see this as really annoying and pointless to an end-user if there are hundreds or thousands of breakpoints. -- Jeff J.