Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded

Vladimir Prus
2008-02-08T06:44:05Z

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", "<PENDING>");
> > > else if (header_of_multiple)
> > > ui_out_field_string (uiout, "addr", "<MULTIPLE>");
> > > + else if (loc->shlib_disabled)
> > > + ui_out_field_string (uiout, "addr", "<PENDING>");
> > > 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", "<PENDING>");
> > - else if (header_of_multiple)
> > + if (header_of_multiple)
> > ui_out_field_string (uiout, "addr", "<MULTIPLE>");
> > + if (b->loc == NULL || loc->shlib_disabled)
> > + ui_out_field_string (uiout, "addr", "<PENDING>");
> > 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