From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7910 invoked by alias); 8 Feb 2008 06:44:05 -0000 Received: (qmail 7899 invoked by uid 22791); 8 Feb 2008 06:44:03 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 08 Feb 2008 06:43:46 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1JNMxV-0006oX-Cn for gdb-patches@sourceware.org; Fri, 08 Feb 2008 09:43:42 +0300 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA:32) (Exim 4.50) id 1JNMwR-0006nZ-00; Fri, 08 Feb 2008 09:42:31 +0300 From: Vladimir Prus To: Nick Roberts Subject: Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded Date: Fri, 08 Feb 2008 06:44:00 -0000 User-Agent: KMail/1.9.6 (enterprise 0.20070907.709405) Cc: Joel Brobecker , gdb-patches@sourceware.org References: <20080204214226.GF20922@adacore.com> <20080207063817.GA3907@adacore.com> <18347.45621.630420.453287@kahikatea.snap.net.nz> In-Reply-To: <18347.45621.630420.453287@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200802080942.29622.ghost@cs.msu.su> 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-02/txt/msg00147.txt.bz2 On Friday 08 February 2008 04:36:53 Nick Roberts wrote: > Joel Brobecker writes: > > Vladimir, > > > > I could use a little help with your feedback. I think I'm getting it, > > but I think you might know more about this part of the code. > > Since it looks like Vladimir can't find the time to look at this Huh? I don't think expecting a response within one business day is quite reasonable. > and because > you might like to make the branch shortly, FWIW, I've looked more closely > at it: > > > I think we're still not there, unfortunately. > > > > > state -- it will be apparent from the locations. */ > > > if (header_of_multiple) > > > pending = 0; > > > ! ui_out_field_fmt (uiout, "enabled", "%c", > > > ! bpenables[(int) b->enable_state]); > > > } > > > > The following code above "ui_out_field_fmt" can be deleted now: > > > > int pending = (b->loc == NULL || b->loc->shlib_disabled); > > /* For header of multiple, there's no point showing pending > > state -- it will be apparent from the locations. */ > > if (header_of_multiple) > > pending = 0; > > > > That means that the curly braces can go too. We also need to restore > > the spacing after the enable state. We end up with: > > I agree. > > > annotate_field (3); > > if (part_of_multiple) > > ui_out_field_string (uiout, "enabled", loc->enabled ? "y" : "n"); > > else > > ui_out_field_fmt (uiout, "enabled", "%c", > > bpenables[(int) b->enable_state]); > > ui_out_spaces (uiout, 2); > > Putting this back to 2 spaces requires a similar change to the header to keep > alignment: > > *************** breakpoint_1 (int bnum, int allflag) > *** 3780,3786 **** > ui_out_table_header (uiout, 4, ui_left, "disp", "Disp"); /* 3 */ > if (nr_printable_breakpoints > 0) > annotate_field (3); > ! ui_out_table_header (uiout, 4, ui_left, "enabled", "Enb"); /* 4 */ > if (addressprint) > { > if (nr_printable_breakpoints > 0) > --- 3767,3773 ---- > ui_out_table_header (uiout, 4, ui_left, "disp", "Disp"); /* 3 */ > if (nr_printable_breakpoints > 0) > annotate_field (3); > ! ui_out_table_header (uiout, 3, ui_left, "enabled", "Enb"); /* 4 */ > if (addressprint) > { > if (nr_printable_breakpoints > 0) > > > > > ui_out_field_string (uiout, "addr", ""); > > > else if (header_of_multiple) > > > ui_out_field_string (uiout, "addr", ""); > > > + else if (loc->shlib_disabled) > > > + ui_out_field_string (uiout, "addr", ""); > > > else > > > ui_out_field_core_addr (uiout, "addr", loc->address); > > > } > > > > I understand what Vladimir said, but I think that the following should > > work too: > > > > @@ -3552,10 +3552,10 @@ print_one_breakpoint_location (struct br > > if (addressprint) > > { > > annotate_field (4); > > - if (b->loc == NULL) > > - ui_out_field_string (uiout, "addr", ""); > > - else if (header_of_multiple) > > + if (header_of_multiple) > > ui_out_field_string (uiout, "addr", ""); > > + if (b->loc == NULL || loc->shlib_disabled) > > + ui_out_field_string (uiout, "addr", ""); > > else > > ui_out_field_core_addr (uiout, "addr", loc->address); > > } > > > > The idea is that you can't have header_of_multiple=1 and b->loc == NULL > > at the same time. So it's OK to move the check for header_of_multiple > > up. And that avoids the code duplication. > > I agree. > > I can confirm that Gdb works as I would expect with these changes. Since you seem to have a patch relative to previous one, would you mind sending a complete patch relative to CVS HEAD? > > On a related note, breakpoint.c now has 7 columns: > > ui_out_table_header (uiout, 7, ui_left, "number", "Num"); /* 1 */ > > up from the previous 3: > > ui_out_table_header (uiout, 3, ui_left, "number", "Num"); /* 1 */ > > presumably for breakpoint numbers like 1.2 etc but it still seems to > anticipate a very large number of breakpoints. That's part of the original multiple location patch; I don't think it's related in any way to what we're discussing now. - Volodya