Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
       [not found] <20080204214226.GF20922@adacore.com>
@ 2008-02-04 21:55 ` Nick Roberts
  2008-02-05  0:12   ` Joel Brobecker
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-04 21:55 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

 > As far as I could tell, the discussions pointed towards the idea
 > of removing the address and replacing it with "<PENDING>". Can we
 > expect to see a patch along these lines sometimes soon?

I think the patch below, based on my initial patch and Vladimir's subsequent
comment, will fix it.  No regressions.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** breakpoint.c	04 Feb 2008 09:10:59 +1300	1.301
--- breakpoint.c	04 Feb 2008 09:11:02 +1300	
*************** print_one_breakpoint_location (struct br
*** 3425,3434 ****
    /* 4 */
    annotate_field (3);
    if (part_of_multiple)
!     ui_out_field_string (uiout, "enabled", 
! 			 loc->shlib_disabled 
! 			 ? (loc->enabled ? "y(p)" : "n(p)")
! 			 : (loc->enabled ? "y" : "n"));
    else
      {
        int pending = (b->loc == NULL || b->loc->shlib_disabled);
--- 3425,3431 ----
    /* 4 */
    annotate_field (3);
    if (part_of_multiple)
!     ui_out_field_string (uiout, "enabled", loc->enabled ? "y" : "n");
    else
      {
        int pending = (b->loc == NULL || b->loc->shlib_disabled);
*************** print_one_breakpoint_location (struct br
*** 3556,3561 ****
--- 3553,3560 ----
  	      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);
  	  }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-04 21:55 ` (gdb-6.8) Discard breakpoint address if shared library is unloaded Nick Roberts
@ 2008-02-05  0:12   ` Joel Brobecker
  2008-02-05  0:21     ` Joel Brobecker
  2008-02-05  0:54     ` Nick Roberts
  0 siblings, 2 replies; 25+ messages in thread
From: Joel Brobecker @ 2008-02-05  0:12 UTC (permalink / raw)
  To: Nick Roberts; +Cc: gdb-patches

> I think the patch below, based on my initial patch and Vladimir's subsequent
> comment, will fix it.  No regressions.

This looks fine. But I'd like to hear at least from Vladimir.

I have a question: Isn't this part going to break your parser as well?


      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;
      ui_out_field_fmt (uiout, "enabled", "%c%s",
                        bpenables[(int) b->enable_state],
                        pending ? "(p)" : "");

This is the counterpart of the branch you modified. As you can see,
it can print the dreaded "(p)" in addition to the y/n enabled state.

> -- 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> *** breakpoint.c	04 Feb 2008 09:10:59 +1300	1.301
> --- breakpoint.c	04 Feb 2008 09:11:02 +1300	
> *************** print_one_breakpoint_location (struct br
> *** 3425,3434 ****
>     /* 4 */
>     annotate_field (3);
>     if (part_of_multiple)
> !     ui_out_field_string (uiout, "enabled", 
> ! 			 loc->shlib_disabled 
> ! 			 ? (loc->enabled ? "y(p)" : "n(p)")
> ! 			 : (loc->enabled ? "y" : "n"));
>     else
>       {
>         int pending = (b->loc == NULL || b->loc->shlib_disabled);
> --- 3425,3431 ----
>     /* 4 */
>     annotate_field (3);
>     if (part_of_multiple)
> !     ui_out_field_string (uiout, "enabled", loc->enabled ? "y" : "n");
>     else
>       {
>         int pending = (b->loc == NULL || b->loc->shlib_disabled);
> *************** print_one_breakpoint_location (struct br
> *** 3556,3561 ****
> --- 3553,3560 ----
>   	      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);
>   	  }

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-05  0:12   ` Joel Brobecker
@ 2008-02-05  0:21     ` Joel Brobecker
  2008-02-05  0:36       ` Nick Roberts
  2008-02-05  0:54     ` Nick Roberts
  1 sibling, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-05  0:21 UTC (permalink / raw)
  To: Nick Roberts; +Cc: gdb-patches

> > *************** print_one_breakpoint_location (struct br
> > *** 3556,3561 ****
> > --- 3553,3560 ----
> >   	      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);

And I forgot. You can also avoid some code duplication by changing:

Index: breakpoint.c
===================================================================
--- breakpoint.c	(revision 146)
+++ breakpoint.c	(working copy)
@@ -3506,7 +3506,7 @@ print_one_breakpoint_location (struct br
 	if (addressprint)
 	  {
 	    annotate_field (4);
-	    if (b->loc == NULL)
+	    if (b->loc == NULL || loc->shlib_disabled)
 	      ui_out_field_string (uiout, "addr", "<PENDING>");
 	    else if (header_of_multiple)
 	      ui_out_field_string (uiout, "addr", "<MULTIPLE>");

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-05  0:21     ` Joel Brobecker
@ 2008-02-05  0:36       ` Nick Roberts
  0 siblings, 0 replies; 25+ messages in thread
From: Nick Roberts @ 2008-02-05  0:36 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

 > And I forgot. You can also avoid some code duplication by changing:
 > 
 > Index: breakpoint.c
 > ===================================================================
 > --- breakpoint.c	(revision 146)
 > +++ breakpoint.c	(working copy)
 > @@ -3506,7 +3506,7 @@ print_one_breakpoint_location (struct br
 >  	if (addressprint)
 >  	  {
 >  	    annotate_field (4);
 > -	    if (b->loc == NULL)
 > +	    if (b->loc == NULL || loc->shlib_disabled)
 >  	      ui_out_field_string (uiout, "addr", "<PENDING>");
 >  	    else if (header_of_multiple)
 >  	      ui_out_field_string (uiout, "addr", "<MULTIPLE>");

That's what I had originally
(http://sourceware.org/ml/gdb-patches/2007-11/msg00262.html)

but Vladimir said:
(http://sourceware.org/ml/gdb-patches/2007-11/msg00266.html)

  I believe that in a case of breakpoint with multiple locations, where
  the first location is shlib_disabled, the updated code will print <PENDING>
  as address, not <MULTIPLE>.

  I think the right approach would be moving the check for loc->shlib_disabled
  later, like this:
  ...

so I did this as he was the author of the original patch.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-05  0:12   ` Joel Brobecker
  2008-02-05  0:21     ` Joel Brobecker
@ 2008-02-05  0:54     ` Nick Roberts
  2008-02-07  6:38       ` Joel Brobecker
  1 sibling, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-05  0:54 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

 > I have a question: Isn't this part going to break your parser as well?

Yes.  It was in my original patch but I seem to have lost it somehow in the
intervening period.  I attach the updated patch.

Thanks.

 >       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;
 >       ui_out_field_fmt (uiout, "enabled", "%c%s",
 >                         bpenables[(int) b->enable_state],
 >                         pending ? "(p)" : "");
 > 
 > This is the counterpart of the branch you modified. As you can see,
 > it can print the dreaded "(p)" in addition to the y/n enabled state.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-02-05  Nick Roberts  <nickrob@snap.net.nz>

	* breakpoint.c (print_one_breakpoint_location): Revert Enb field
	to old format.  Discard breakpoint address if shared library is
	unloaded.



*** breakpoint.c	04 Feb 2008 09:10:59 +1300	1.301
--- breakpoint.c	05 Feb 2008 13:46:40 +1300	
*************** print_one_breakpoint_location (struct br
*** 3425,3434 ****
    /* 4 */
    annotate_field (3);
    if (part_of_multiple)
!     ui_out_field_string (uiout, "enabled", 
! 			 loc->shlib_disabled 
! 			 ? (loc->enabled ? "y(p)" : "n(p)")
! 			 : (loc->enabled ? "y" : "n"));
    else
      {
        int pending = (b->loc == NULL || b->loc->shlib_disabled);
--- 3425,3431 ----
    /* 4 */
    annotate_field (3);
    if (part_of_multiple)
!     ui_out_field_string (uiout, "enabled", loc->enabled ? "y" : "n");
    else
      {
        int pending = (b->loc == NULL || b->loc->shlib_disabled);
*************** print_one_breakpoint_location (struct br
*** 3436,3446 ****
  	 state -- it will be apparent from the locations.  */
        if (header_of_multiple)
  	pending = 0;
!       ui_out_field_fmt (uiout, "enabled", "%c%s", 
! 			bpenables[(int) b->enable_state],
! 			pending ? "(p)" : "");
!       if (!pending)
! 	ui_out_spaces (uiout, 3);
      }
  
    
--- 3433,3440 ----
  	 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]);
      }
  
    
*************** print_one_breakpoint_location (struct br
*** 3556,3561 ****
--- 3550,3557 ----
  	      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);
  	  }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-05  0:54     ` Nick Roberts
@ 2008-02-07  6:38       ` Joel Brobecker
  2008-02-08  1:37         ` Nick Roberts
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-07  6:38 UTC (permalink / raw)
  To: Nick Roberts, ghost; +Cc: gdb-patches

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.

> Yes.  It was in my original patch but I seem to have lost it somehow in the
> intervening period.  I attach the updated patch.

Oh, good.

> 2008-02-05  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* breakpoint.c (print_one_breakpoint_location): Revert Enb field
> 	to old format.  Discard breakpoint address if shared library is
> 	unloaded.

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:

  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);

>   	      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.

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-07  6:38       ` Joel Brobecker
@ 2008-02-08  1:37         ` Nick Roberts
  2008-02-08  6:44           ` Vladimir Prus
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-08  1:37 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: ghost, gdb-patches

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 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.

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.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-08  1:37         ` Nick Roberts
@ 2008-02-08  6:44           ` Vladimir Prus
  2008-02-08  7:37             ` Nick Roberts
  0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Prus @ 2008-02-08  6:44 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Joel Brobecker, gdb-patches

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


 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-08  6:44           ` Vladimir Prus
@ 2008-02-08  7:37             ` Nick Roberts
  2008-02-14 21:43               ` Joel Brobecker
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-08  7:37 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: Joel Brobecker, gdb-patches

 > > 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.

I mean since last November when I first raised the issue.

 >...
 > >  > 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?

Joel's version is relative to CVS HEAD.  All I'm suggesting is that also
requires that the line

   ui_out_table_header (uiout, 4, ui_left, "enabled", "Enb");	/* 4 */

is changed back to

   ui_out_table_header (uiout, 3, ui_left, "enabled", "Enb");	/* 4 */

 > > 
 > > 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.

Granted, it's not central to the issue at hand but it's related in that it
also involves formatting of the header of the breakpoints table and was
made on the same day.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-08  7:37             ` Nick Roberts
@ 2008-02-14 21:43               ` Joel Brobecker
  2008-02-15  4:01                 ` Nick Roberts
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-14 21:43 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Vladimir Prus, gdb-patches

Nick,

>  > Since you seem to have a patch relative to previous one, would you mind
>  > sending a complete patch relative to CVS HEAD?
> 
> Joel's version is relative to CVS HEAD.  All I'm suggesting is that also
> requires that the line

Can you please resend a new complete patch, with a ChangeLog, and I will
review it. It's much easier that way for me to make sure that what I
think you are proposing is indeed what you are proposing to commit.

Thanks,
-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-14 21:43               ` Joel Brobecker
@ 2008-02-15  4:01                 ` Nick Roberts
  2008-02-15  8:39                   ` Eli Zaretskii
                                     ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Nick Roberts @ 2008-02-15  4:01 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Vladimir Prus, gdb-patches

 > Can you please resend a new complete patch, with a ChangeLog, and I will
 > review it. It's much easier that way for me to make sure that what I
 > think you are proposing is indeed what you are proposing to commit.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-02-15  Nick Roberts  <nickrob@snap.net.nz>

	* breakpoint.c (print_one_breakpoint_location): Revert Enb field
	to old format.  Discard breakpoint address if shared library is
	unloaded.
	(breakpoint_1): Adjust formatting of table header accordingly.



*** breakpoint.c	15 Feb 2008 16:52:37 +1300	1.302
--- breakpoint.c	15 Feb 2008 16:55:43 +1300	
*************** print_one_breakpoint_location (struct br
*** 3426,3448 ****
    /* 4 */
    annotate_field (3);
    if (part_of_multiple)
!     ui_out_field_string (uiout, "enabled", 
! 			 loc->shlib_disabled 
! 			 ? (loc->enabled ? "y(p)" : "n(p)")
! 			 : (loc->enabled ? "y" : "n"));
    else
!     {
!       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;
!       ui_out_field_fmt (uiout, "enabled", "%c%s", 
! 			bpenables[(int) b->enable_state],
! 			pending ? "(p)" : "");
!       if (!pending)
! 	ui_out_spaces (uiout, 3);
!     }
  
    
    /* 5 and 6 */
--- 3426,3436 ----
    /* 4 */
    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);
  
    
    /* 5 and 6 */
*************** print_one_breakpoint_location (struct br
*** 3553,3562 ****
  	if (addressprint)
  	  {
  	    annotate_field (4);
! 	    if (b->loc == NULL)
! 	      ui_out_field_string (uiout, "addr", "<PENDING>");
! 	    else if (header_of_multiple)
  	      ui_out_field_string (uiout, "addr", "<MULTIPLE>");
  	    else
  	      ui_out_field_core_addr (uiout, "addr", loc->address);
  	  }
--- 3541,3550 ----
  	if (addressprint)
  	  {
  	    annotate_field (4);
! 	    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);
  	  }
*************** breakpoint_1 (int bnum, int allflag)
*** 3781,3787 ****
    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)
--- 3769,3775 ----
    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)


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-15  4:01                 ` Nick Roberts
@ 2008-02-15  8:39                   ` Eli Zaretskii
  2008-02-15  9:13                     ` Nick Roberts
  2008-02-17  9:56                   ` Vladimir Prus
  2008-02-19 19:02                   ` Joel Brobecker
  2 siblings, 1 reply; 25+ messages in thread
From: Eli Zaretskii @ 2008-02-15  8:39 UTC (permalink / raw)
  To: Nick Roberts; +Cc: brobecker, ghost, gdb-patches

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Fri, 15 Feb 2008 17:01:00 +1300
> Cc: Vladimir Prus <ghost@cs.msu.su>, gdb-patches@sourceware.org
> 
> *** breakpoint.c	15 Feb 2008 16:52:37 +1300	1.302
> --- breakpoint.c	15 Feb 2008 16:55:43 +1300	
> *************** print_one_breakpoint_location (struct br
> *** 3426,3448 ****
>     /* 4 */
>     annotate_field (3);
>     if (part_of_multiple)
> !     ui_out_field_string (uiout, "enabled", 
> ! 			 loc->shlib_disabled 
> ! 			 ? (loc->enabled ? "y(p)" : "n(p)")
> ! 			 : (loc->enabled ? "y" : "n"));
>     else
> !     {
> !       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;
> !       ui_out_field_fmt (uiout, "enabled", "%c%s", 
> ! 			bpenables[(int) b->enable_state],
> ! 			pending ? "(p)" : "");
> !       if (!pending)
> ! 	ui_out_spaces (uiout, 3);
> !     }
>   
>     
>     /* 5 and 6 */
> --- 3426,3436 ----
>     /* 4 */
>     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);
>   
>     
>     /* 5 and 6 */
> *************** print_one_breakpoint_location (struct br
> *** 3553,3562 ****
>   	if (addressprint)
>   	  {
>   	    annotate_field (4);
> ! 	    if (b->loc == NULL)
> ! 	      ui_out_field_string (uiout, "addr", "<PENDING>");
> ! 	    else if (header_of_multiple)
>   	      ui_out_field_string (uiout, "addr", "<MULTIPLE>");
>   	    else
>   	      ui_out_field_core_addr (uiout, "addr", loc->address);
>   	  }
> --- 3541,3550 ----
>   	if (addressprint)
>   	  {
>   	    annotate_field (4);
> ! 	    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);
>   	  }
> *************** breakpoint_1 (int bnum, int allflag)
> *** 3781,3787 ****
>     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)
> --- 3769,3775 ----
>     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)

This will require a suitable change to the manual, right?


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-15  8:39                   ` Eli Zaretskii
@ 2008-02-15  9:13                     ` Nick Roberts
  2008-02-16 12:59                       ` Eli Zaretskii
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-15  9:13 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: brobecker, ghost, gdb-patches

 > This will require a suitable change to the manual, right?

This patch just reverts the description of "Enabled or Disabled" column of
the breakpoint table and moves the revised sentence about pending
breakpoints to back to the Address column (which already has some description of
them).

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-02-15  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.texinfo (Set Breaks): Revert description of Enb column of
	breakpoint table.



*** gdb.texinfo.~1.467.~	2008-02-04 08:48:31.000000000 +1300
--- gdb.texinfo	2008-02-15 22:05:02.000000000 +1300
*************** Breakpoint, watchpoint, or catchpoint.
*** 2987,3002 ****
  Whether the breakpoint is marked to be disabled or deleted when hit.
  @item Enabled or Disabled
  Enabled breakpoints are marked with @samp{y}.  @samp{n} marks breakpoints
! that are not enabled.  An optional @samp{(p)} suffix marks pending
! breakpoints---breakpoints for which address is either not yet
! resolved, pending load of a shared library, or for which address was
! in a shared library that was since unloaded.  Such breakpoint won't
! fire until a shared library that has the symbol or line referred by
! breakpoint is loaded.  See below for details.
  @item Address
  Where the breakpoint is in your program, as a memory address.  For a
! pending breakpoint whose address is not yet known,  this field will
! contain @samp{<PENDING>}.  A breakpoint with several locations will
  have @samp{<MULTIPLE>} in this field---see below for details.
  @item What
  Where the breakpoint is in the source for your program, as a file and
--- 2987,2999 ----
  Whether the breakpoint is marked to be disabled or deleted when hit.
  @item Enabled or Disabled
  Enabled breakpoints are marked with @samp{y}.  @samp{n} marks breakpoints
! that are not enabled.
  @item Address
  Where the breakpoint is in your program, as a memory address.  For a
! pending breakpoint whose address is not yet known, this field will
! contain @samp{<PENDING>}.  Such breakpoint won't fire until a shared
! library that has the symbol or line referred by breakpoint is loaded.
! See below for details.  A breakpoint with several locations will
  have @samp{<MULTIPLE>} in this field---see below for details.
  @item What
  Where the breakpoint is in the source for your program, as a file and


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-15  9:13                     ` Nick Roberts
@ 2008-02-16 12:59                       ` Eli Zaretskii
  0 siblings, 0 replies; 25+ messages in thread
From: Eli Zaretskii @ 2008-02-16 12:59 UTC (permalink / raw)
  To: Nick Roberts; +Cc: brobecker, ghost, gdb-patches

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Fri, 15 Feb 2008 22:13:00 +1300
> Cc: brobecker@adacore.com, ghost@cs.msu.su,
> 	gdb-patches@sourceware.org
> 
>  > This will require a suitable change to the manual, right?
> 
> This patch just reverts the description of "Enabled or Disabled" column of
> the breakpoint table and moves the revised sentence about pending
> breakpoints to back to the Address column (which already has some description of
> them).
> 
> -- 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> 2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* gdb.texinfo (Set Breaks): Revert description of Enb column of
> 	breakpoint table.

Fine with me, thanks.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-15  4:01                 ` Nick Roberts
  2008-02-15  8:39                   ` Eli Zaretskii
@ 2008-02-17  9:56                   ` Vladimir Prus
  2008-02-17 19:53                     ` Nick Roberts
  2008-02-19 19:02                   ` Joel Brobecker
  2 siblings, 1 reply; 25+ messages in thread
From: Vladimir Prus @ 2008-02-17  9:56 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Joel Brobecker, gdb-patches

On Friday 15 February 2008 07:01:00 Nick Roberts wrote:
>  > Can you please resend a new complete patch, with a ChangeLog, and I will
>  > review it. It's much easier that way for me to make sure that what I
>  > think you are proposing is indeed what you are proposing to commit.
> 
> -- 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> 2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * breakpoint.c (print_one_breakpoint_location): Revert Enb field
>         to old format.  Discard breakpoint address if shared library is
>         unloaded.
>         (breakpoint_1): Adjust formatting of table header accordingly.
> 
> 
> 
> *** breakpoint.c        15 Feb 2008 16:52:37 +1300      1.302
> --- breakpoint.c        15 Feb 2008 16:55:43 +1300      
> *************** print_one_breakpoint_location (struct br
> *** 3426,3448 ****
>     /* 4 */
>     annotate_field (3);
>     if (part_of_multiple)
> !     ui_out_field_string (uiout, "enabled", 
> !                        loc->shlib_disabled 
> !                        ? (loc->enabled ? "y(p)" : "n(p)")
> !                        : (loc->enabled ? "y" : "n"));
>     else
> !     {
> !       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;
> !       ui_out_field_fmt (uiout, "enabled", "%c%s", 
> !                       bpenables[(int) b->enable_state],
> !                       pending ? "(p)" : "");
> !       if (!pending)
> !       ui_out_spaces (uiout, 3);
> !     }
>   
>     
>     /* 5 and 6 */
> --- 3426,3436 ----
>     /* 4 */
>     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);
>   
>     
>     /* 5 and 6 */
> *************** print_one_breakpoint_location (struct br
> *** 3553,3562 ****
>         if (addressprint)
>           {
>             annotate_field (4);
> !           if (b->loc == NULL)
> !             ui_out_field_string (uiout, "addr", "<PENDING>");
> !           else if (header_of_multiple)
>               ui_out_field_string (uiout, "addr", "<MULTIPLE>");
>             else
>               ui_out_field_core_addr (uiout, "addr", loc->address);
>           }
> --- 3541,3550 ----
>         if (addressprint)
>           {
>             annotate_field (4);
> !           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);
>           }
> *************** breakpoint_1 (int bnum, int allflag)
> *** 3781,3787 ****
>     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)
> --- 3769,3775 ----
>     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)

Did you run the testsuite with this patch? I'm getting the following test failures
if the patch is applied:


FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after setting one-by-one)
FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after cancel)
FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after setting on all)

I did not run the entire testsuite, only gdb.cp directory, so there maybe
be more.

While looking on the patch I've noticed a pre-existing problem -- say
we create a pending breakpoint of a function called foo. Then 'info break'
will say the breakpoint is on 'foo'. After the breakpoint is resolved,
info break will mention both the function name and the source line.
If the library is unloaded, then info break will only mention source line ---
even though breakpoint's address_string actually refers to function.

I don't think we should bother to fix this right now, as the code in
question seems rather old.

Other than that, I did not noticed any issues, so I presume the patch can
go in as soon as the test failures are handled.

- Volodya


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-17  9:56                   ` Vladimir Prus
@ 2008-02-17 19:53                     ` Nick Roberts
  0 siblings, 0 replies; 25+ messages in thread
From: Nick Roberts @ 2008-02-17 19:53 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: Joel Brobecker, gdb-patches

 > FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after setting one-by-one)
 > FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after cancel)
 > FAIL: gdb.cp/ovldbreak.exp: breakpoint info (after setting on all)

That's right.  It just needs the changes to ovldbreak.exp to be reverted
(2007-09-23?) to look for the old header format (one less space in "Enb"
columnn)

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-15  4:01                 ` Nick Roberts
  2008-02-15  8:39                   ` Eli Zaretskii
  2008-02-17  9:56                   ` Vladimir Prus
@ 2008-02-19 19:02                   ` Joel Brobecker
  2008-02-19 20:04                     ` Nick Roberts
  2 siblings, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-19 19:02 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Vladimir Prus, gdb-patches

Nick,

> 2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* breakpoint.c (print_one_breakpoint_location): Revert Enb field
> 	to old format.  Discard breakpoint address if shared library is
> 	unloaded.
> 	(breakpoint_1): Adjust formatting of table header accordingly.

The patch looks fine, but I assumed that you had run the testsuite
before sending it. From my end, it looks like you didn't, and I find
this quite disappointing. Based on Volodya's email, I ran the testsuite
myself (which I really shouldn't have to), and discovered that your
patch causes ~20 new failures, not just the 3 that Volodya discovered.

Your patch cannot go in until the failures are investigated and fixed.

Please make sure to always confirm in your submissions that you did
run the testsuite, mentioning which architecture it was run on, and
also confirm that it did not introduce any regression.

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-19 19:02                   ` Joel Brobecker
@ 2008-02-19 20:04                     ` Nick Roberts
  2008-02-20 16:31                       ` Joel Brobecker
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-19 20:04 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Vladimir Prus, gdb-patches

Joel Brobecker writes:
 > Nick,
 > 
 > > 2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > 	* breakpoint.c (print_one_breakpoint_location): Revert Enb field
 > > 	to old format.  Discard breakpoint address if shared library is
 > > 	unloaded.
 > > 	(breakpoint_1): Adjust formatting of table header accordingly.
 > 
 > The patch looks fine, but I assumed that you had run the testsuite
 > before sending it. From my end, it looks like you didn't, and I find
 > this quite disappointing. Based on Volodya's email, I ran the testsuite
 > myself (which I really shouldn't have to), and discovered that your
 > patch causes ~20 new failures, not just the 3 that Volodya discovered.
 > 
 > Your patch cannot go in until the failures are investigated and fixed.
 > 
 > Please make sure to always confirm in your submissions that you did
 > run the testsuite, mentioning which architecture it was run on, and
 > also confirm that it did not introduce any regression.

I did run the whole testsuite on the first patch that I submitted which, as you
pointed out, was missing some detail which I had somehow lost since last
November, but not the amendment to that patch.  You might find it disappointing
that I've not rerun the testsuite but I find it disappointing that I have had
to submit complete patches for code, doumentation and testsuite to revert
changes which I pointed out broke things for Emacs.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-19 20:04                     ` Nick Roberts
@ 2008-02-20 16:31                       ` Joel Brobecker
  2008-02-20 19:21                         ` Nick Roberts
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-20 16:31 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Vladimir Prus, gdb-patches

Nick,

> You might find it disappointing that I've not rerun the testsuite but
> I find it disappointing that I have had to submit complete patches for
> code, doumentation and testsuite to revert changes which I pointed out
> broke things for Emacs.

Here is my point of view. It's my personal opinion on it and others
may have a different view on this, but FWIW: IMO, the reason why
a change "broke" emacs is because the GUD mode in emacs made some
undocumented assumptions on the format of that ouput of one of
the CLI commands. I recognize the importance of staying compatible,
particularly in a case like this where it seems relatively easy
to achieve. But you have to understand that nowhere did we promise
that we would make the CLI output "compatible". But again, we'll
do our best.

I have been more than willing to work with you on this, by delaying
the release and reviewing your patches, but I think we'll save each
other a lot of suffering if you accept that you are now in charge
of fixing this issue without trying to determine whether it should
be you or someone else that should be doing this.

So how about you submit a complete patch, following the usual protocol
rather than an abbreviated one? I'll review it promptly, and we can
put this episode behind us. As I said before, I reviewed the breakpoint.c
patch and it looked fine, so it's just a matter of taking a look at
the testsuite failures and adjust the testsuite accordingly (if
justified).

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-20 16:31                       ` Joel Brobecker
@ 2008-02-20 19:21                         ` Nick Roberts
  2008-02-20 20:27                           ` Vladimir Prus
  0 siblings, 1 reply; 25+ messages in thread
From: Nick Roberts @ 2008-02-20 19:21 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Vladimir Prus, gdb-patches

 > So how about you submit a complete patch, following the usual protocol
 > rather than an abbreviated one? I'll review it promptly, and we can
 > put this episode behind us. As I said before, I reviewed the breakpoint.c
 > patch and it looked fine, so it's just a matter of taking a look at
 > the testsuite failures and adjust the testsuite accordingly (if
 > justified).

Some or all of the other failures occur in these files:

2007-09-23  Vladimir Prus  <vladimir@codesourcery.com>
	
	* gdb.base/annota1.exp: Adjust for 'info break'
	format changes.
	* gdb.base/annota3.exp: Likewise.
	* gdb.base/break.exp: Likewise.
	* gdb.base/condbreak.exp: Likewise.
	* gdb.base/pending.exp: Likewise.
	* gdb.base/sepdebug.exp: Likewise.
	* gdb.base/unload.exp: Likewise.
	* gdb.base/ovldbreak.exp: Likewise.


So I'm surprised that Vladimir did not know there would be more than those
in ovldbreak.exp (actually cp.base).

It is possible for me to try to understand each of these changes and revert
them but clearly it is far easier for Vladimir to do that as he made the
changes in the first place.

I will try to do that but I don't have the time at the moment.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-20 19:21                         ` Nick Roberts
@ 2008-02-20 20:27                           ` Vladimir Prus
  2008-02-25 10:04                             ` Vladimir Prus
  0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Prus @ 2008-02-20 20:27 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Joel Brobecker, gdb-patches

On Wednesday 20 February 2008 22:20:51 Nick Roberts wrote:
>  > So how about you submit a complete patch, following the usual protocol
>  > rather than an abbreviated one? I'll review it promptly, and we can
>  > put this episode behind us. As I said before, I reviewed the breakpoint.c
>  > patch and it looked fine, so it's just a matter of taking a look at
>  > the testsuite failures and adjust the testsuite accordingly (if
>  > justified).
> 
> Some or all of the other failures occur in these files:
> 
> 2007-09-23  Vladimir Prus  <vladimir@codesourcery.com>
> 	
> 	* gdb.base/annota1.exp: Adjust for 'info break'
> 	format changes.
> 	* gdb.base/annota3.exp: Likewise.
> 	* gdb.base/break.exp: Likewise.
> 	* gdb.base/condbreak.exp: Likewise.
> 	* gdb.base/pending.exp: Likewise.
> 	* gdb.base/sepdebug.exp: Likewise.
> 	* gdb.base/unload.exp: Likewise.
> 	* gdb.base/ovldbreak.exp: Likewise.
> 
> 
> So I'm surprised that Vladimir did not know there would be more than those
> in ovldbreak.exp (actually cp.base).

In my email, I've actually said I did not run the other tests and 
there might be more failures.  Getting the complete list is only
possible by running the testsuite, since I have no idea what tests were
added since then.

> It is possible for me to try to understand each of these changes and revert
> them but clearly it is far easier for Vladimir to do that as he made the
> changes in the first place.
> 
> I will try to do that but I don't have the time at the moment.

So, given that we want to release 6.8 soon, and already spent considerable time
on this patch, it looks like I get to pick up where you left. I'll try to
update the test tomorrow.

- Volodya


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-20 20:27                           ` Vladimir Prus
@ 2008-02-25 10:04                             ` Vladimir Prus
  2008-02-25 19:39                               ` Joel Brobecker
  0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Prus @ 2008-02-25 10:04 UTC (permalink / raw)
  To: Nick Roberts; +Cc: Joel Brobecker, gdb-patches

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

On Wednesday 20 February 2008 23:25:50 Vladimir Prus wrote:
> On Wednesday 20 February 2008 22:20:51 Nick Roberts wrote:
> >  > So how about you submit a complete patch, following the usual protocol
> >  > rather than an abbreviated one? I'll review it promptly, and we can
> >  > put this episode behind us. As I said before, I reviewed the breakpoint.c
> >  > patch and it looked fine, so it's just a matter of taking a look at
> >  > the testsuite failures and adjust the testsuite accordingly (if
> >  > justified).
> > 
> > Some or all of the other failures occur in these files:
> > 
> > 2007-09-23  Vladimir Prus  <vladimir@codesourcery.com>
> > 	
> > 	* gdb.base/annota1.exp: Adjust for 'info break'
> > 	format changes.
> > 	* gdb.base/annota3.exp: Likewise.
> > 	* gdb.base/break.exp: Likewise.
> > 	* gdb.base/condbreak.exp: Likewise.
> > 	* gdb.base/pending.exp: Likewise.
> > 	* gdb.base/sepdebug.exp: Likewise.
> > 	* gdb.base/unload.exp: Likewise.
> > 	* gdb.base/ovldbreak.exp: Likewise.
> > 
> > 
> > So I'm surprised that Vladimir did not know there would be more than those
> > in ovldbreak.exp (actually cp.base).
> 
> In my email, I've actually said I did not run the other tests and 
> there might be more failures.  Getting the complete list is only
> possible by running the testsuite, since I have no idea what tests were
> added since then.
> 
> > It is possible for me to try to understand each of these changes and revert
> > them but clearly it is far easier for Vladimir to do that as he made the
> > changes in the first place.
> > 
> > I will try to do that but I don't have the time at the moment.
> 
> So, given that we want to release 6.8 soon, and already spent considerable time
> on this patch, it looks like I get to pick up where you left. I'll try to
> update the test tomorrow.

I'm sorry that "tomorrow" did not happen. However, here's a patch with
adjusted tests, I don't see any regressions on x86. OK?

- Volodya

    [gdb]
    2008-02-15  Nick Roberts  <nickrob@snap.net.nz>

            * breakpoint.c (print_one_breakpoint_location): Revert Enb field
            to old format.  Discard breakpoint address if shared library is
            unloaded.
            (breakpoint_1): Adjust formatting of table header accordingly.

    [gdb/testsuite]
    2008-02-25  Vladimir Prus  <vladimir@codesourcery.com>

        * gdb.base/annota1.exp: Adjust for 'info break'
        format changes.
        * gdb.base/annota3.exp: Likewise.
        * gdb.base/break.exp: Likewise.
        * gdb.base/condbreak.exp: Likewise.
        * gdb.base/pending.exp: Likewise.
        * gdb.base/sepdebug.exp: Likewise.
        * gdb.base/unload.exp: Likewise.
        * gdb.cp/ovldbreak.exp: Likewise.
        * gdb.mi/mi-pending.exp: Likewise.

[-- Attachment #2: 0001-gdb.patch --]
[-- Type: text/x-diff, Size: 18150 bytes --]

From d8bc6e9ac200e930858cc192d7b0e674be2baeb9 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Mon, 25 Feb 2008 12:02:10 +0300
Subject: [RFA] [gdb]
 2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

        * breakpoint.c (print_one_breakpoint_location): Revert Enb field
        to old format.  Discard breakpoint address if shared library is
        unloaded.
        (breakpoint_1): Adjust formatting of table header accordingly.

[gdb/testsuite]
2008-02-25  Vladimir Prus  <vladimir@codesourcery.com>

	* gdb.base/annota1.exp: Adjust for 'info break'
	format changes.
	* gdb.base/annota3.exp: Likewise.
	* gdb.base/break.exp: Likewise.
	* gdb.base/condbreak.exp: Likewise.
	* gdb.base/pending.exp: Likewise.
	* gdb.base/sepdebug.exp: Likewise.
	* gdb.base/unload.exp: Likewise.
	* gdb.cp/ovldbreak.exp: Likewise.
	* gdb.mi/mi-pending.exp: Likewise.
---
 gdb/breakpoint.c                     |   28 +++++++-----------------
 gdb/testsuite/gdb.base/annota1.exp   |    4 +-
 gdb/testsuite/gdb.base/annota3.exp   |    4 +-
 gdb/testsuite/gdb.base/break.exp     |    4 +-
 gdb/testsuite/gdb.base/condbreak.exp |    2 +-
 gdb/testsuite/gdb.base/pending.exp   |   38 +++++++++++++++++-----------------
 gdb/testsuite/gdb.base/sepdebug.exp  |    4 +-
 gdb/testsuite/gdb.base/unload.exp    |    4 +-
 gdb/testsuite/gdb.cp/ovldbreak.exp   |    6 ++--
 gdb/testsuite/gdb.mi/mi-pending.exp  |    2 +-
 10 files changed, 42 insertions(+), 54 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index dca4ca1..019f4c8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3426,23 +3426,11 @@ print_one_breakpoint_location (struct breakpoint *b,
   /* 4 */
   annotate_field (3);
   if (part_of_multiple)
-    ui_out_field_string (uiout, "enabled", 
-			 loc->shlib_disabled 
-			 ? (loc->enabled ? "y(p)" : "n(p)")
-			 : (loc->enabled ? "y" : "n"));
+    ui_out_field_string (uiout, "enabled", loc->enabled ? "y" : "n");
   else
-    {
-      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;
-      ui_out_field_fmt (uiout, "enabled", "%c%s", 
-			bpenables[(int) b->enable_state],
-			pending ? "(p)" : "");
-      if (!pending)
-	ui_out_spaces (uiout, 3);
-    }
+      ui_out_field_fmt (uiout, "enabled", "%c", 
+ 			bpenables[(int) b->enable_state]);
+  ui_out_spaces (uiout, 2);
 
   
   /* 5 and 6 */
@@ -3553,10 +3541,10 @@ print_one_breakpoint_location (struct breakpoint *b,
 	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);
 	  }
@@ -3781,7 +3769,7 @@ breakpoint_1 (int bnum, int allflag)
   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 */
+  ui_out_table_header (uiout, 3, ui_left, "enabled", "Enb");	/* 4 */
   if (addressprint)
 	{
 	  if (nr_printable_breakpoints > 0)
diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index e33aba6..522fdb4 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -117,9 +117,9 @@ gdb_expect {
 #
 send_gdb "info break\n" 
 gdb_expect {
-    -re "\r\n\032\032post-prompt\r\n\r\n\032\032breakpoints-headers\r\n\r\n\032\032field 0\r\nNum     \r\n\032\032field 1\r\nType           \r\n\032\032field 2\r\nDisp \r\n\032\032field 3\r\nEnb  \r\n\032\032field 4\r\nAddress    +\r\n\032\032field 5\r\nWhat\r\n\r\n\032\032breakpoints-table\r\n\r\n\032\032record\r\n\r\n\032\032field 0\r\n1       \r\n\032\032field 1\r\nbreakpoint     \r\n\032\032field 2\r\nkeep \r\n\032\032field 3\r\ny    \r\n\032\032field 4\r\n$hex +\r\n\032\032field 5\r\nin main at ${escapedsrcfile}:$main_line\r\n\r\n\032\032breakpoints-table-end\r\n$gdb_prompt$" \
+    -re "\r\n\032\032post-prompt\r\n\r\n\032\032breakpoints-headers\r\n\r\n\032\032field 0\r\nNum     \r\n\032\032field 1\r\nType           \r\n\032\032field 2\r\nDisp \r\n\032\032field 3\r\nEnb \r\n\032\032field 4\r\nAddress    +\r\n\032\032field 5\r\nWhat\r\n\r\n\032\032breakpoints-table\r\n\r\n\032\032record\r\n\r\n\032\032field 0\r\n1       \r\n\032\032field 1\r\nbreakpoint     \r\n\032\032field 2\r\nkeep \r\n\032\032field 3\r\ny   \r\n\032\032field 4\r\n$hex +\r\n\032\032field 5\r\nin main at ${escapedsrcfile}:$main_line\r\n\r\n\032\032breakpoints-table-end\r\n$gdb_prompt$" \
 	{pass "breakpoint info"}
-    -re "\r\n\032\032post-prompt\r\n\r\n\032\032breakpoints-headers\r\n\r\n\032\032field 0\r\nNum     \r\n\032\032field 1\r\nType           \r\n\032\032field 2\r\nDisp \r\n\032\032field 3\r\nEnb  \r\n\032\032field 4\r\nAddress    +\r\n\032\032field 5\r\nWhat\r\n\r\n\032\032breakpoints-table\r\n\r\n\032\032record\r\n\r\n\032\032field 0\r\n1       \r\n\032\032field 1\r\nbreakpoint     \r\n\032\032field 2\r\nkeep \r\n\032\032field 3\r\ny    \r\n\032\032field 4\r\n$hex +\r\n\032\032field 5\r\nin main at .*${srcfile}:$main_line\r\n\r\n\032\032breakpoints-table-end\r\n$gdb_prompt$" \
+    -re "\r\n\032\032post-prompt\r\n\r\n\032\032breakpoints-headers\r\n\r\n\032\032field 0\r\nNum     \r\n\032\032field 1\r\nType           \r\n\032\032field 2\r\nDisp \r\n\032\032field 3\r\nEnb \r\n\032\032field 4\r\nAddress    +\r\n\032\032field 5\r\nWhat\r\n\r\n\032\032breakpoints-table\r\n\r\n\032\032record\r\n\r\n\032\032field 0\r\n1       \r\n\032\032field 1\r\nbreakpoint     \r\n\032\032field 2\r\nkeep \r\n\032\032field 3\r\ny   \r\n\032\032field 4\r\n$hex +\r\n\032\032field 5\r\nin main at .*${srcfile}:$main_line\r\n\r\n\032\032breakpoints-table-end\r\n$gdb_prompt$" \
 	{ setup_xfail "*-*-*" 1270
           fail "breakpoint info"}
    -re ".*$gdb_prompt$" { fail "breakpoint info" }
diff --git a/gdb/testsuite/gdb.base/annota3.exp b/gdb/testsuite/gdb.base/annota3.exp
index 7bb1fe8..2e0595c 100644
--- a/gdb/testsuite/gdb.base/annota3.exp
+++ b/gdb/testsuite/gdb.base/annota3.exp
@@ -127,8 +127,8 @@ gdb_expect {
 send_gdb "info break\n" 
 gdb_expect_list "breakpoint info" "$gdb_prompt$" {
     "\r\n\032\032post-prompt\r\n"
-    "Num     Type           Disp Enb  Address    +What\r\n"
-    "1       breakpoint     keep y    0x\[0-9a-zA-Z\]+ +in main at .*annota3.c:32\r\n"
+    "Num     Type           Disp Enb Address    +What\r\n"
+    "1       breakpoint     keep y   0x\[0-9a-zA-Z\]+ +in main at .*annota3.c:32\r\n"
 }
 
 
diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
index 6da8203..0ec70c9 100644
--- a/gdb/testsuite/gdb.base/break.exp
+++ b/gdb/testsuite/gdb.base/break.exp
@@ -179,7 +179,7 @@ set bp_location8 [gdb_get_line_number "set breakpoint 8 here" $srcfile1]
 set bp_location9 [gdb_get_line_number "set breakpoint 9 here" $srcfile1]
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$main_line.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in marker2 at .*$srcfile1:($bp_location8|$bp_location9).*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in factorial$proto at .*$srcfile:$bp_location7.*
@@ -309,7 +309,7 @@ gdb_test  "tbreak $srcfile:$bp_location11" "Breakpoint.*at.* file .*$srcfile, li
 #
 # check to see what breakpoints are set (temporary this time)
 #
-gdb_test "info break" "Num     Type.*Disp Enb  Address.*What.*\[\r\n\]
+gdb_test "info break" "Num     Type.*Disp Enb Address.*What.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in main at .*$srcfile:$main_line.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in factorial$proto at .*$srcfile:$bp_location7.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in main at .*$srcfile:$bp_location1.*\[\r\n\]
diff --git a/gdb/testsuite/gdb.base/condbreak.exp b/gdb/testsuite/gdb.base/condbreak.exp
index 6f210dc..e76605c 100644
--- a/gdb/testsuite/gdb.base/condbreak.exp
+++ b/gdb/testsuite/gdb.base/condbreak.exp
@@ -125,7 +125,7 @@ if {$hp_aCC_compiler} {
 }
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$bp_location6.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in marker1$marker1_proto at .*$srcfile1:($bp_location15|$bp_location16).*
 \[\t \]+stop only if \\(1==1\\).*
diff --git a/gdb/testsuite/gdb.base/pending.exp b/gdb/testsuite/gdb.base/pending.exp
index 464a395..bcca61e 100644
--- a/gdb/testsuite/gdb.base/pending.exp
+++ b/gdb/testsuite/gdb.base/pending.exp
@@ -71,8 +71,8 @@ gdb_test_multiple "break pendfunc1" "set pending breakpoint" {
 }
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendfunc1.*" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*" \
 "single pending breakpoint info"
 
 #
@@ -86,8 +86,8 @@ gdb_test "break main" \
     "breakpoint function"
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline" \
 "pending plus real breakpoint info"
 
@@ -108,8 +108,8 @@ gdb_test_multiple "break pendfunc2" "Don't set pending breakpoint" {
 gdb_test "condition 1 k == 1" ""
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*
 \[\t \]+stop only if k == 1.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline" \
 "pending plus condition"
@@ -121,8 +121,8 @@ gdb_test "info break" \
 gdb_test "disable 1" ""
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep n\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
 \[\t \]+stop only if k == 1.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline" \
 "pending disabled"
@@ -134,8 +134,8 @@ gdb_test "commands 1\nprint k\nend" "" \
         "Set commands for pending breakpoint"
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep n\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
 \[\t \]+stop only if k == 1.*
 \[\t \]+print k.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline" \
@@ -154,12 +154,12 @@ gdb_test_multiple "break pendshr.c:$bp2_loc if x > 3" "Set pending breakpoint 2"
 }
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep n\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
 \[\t \]+stop only if k == 1.*
 \[\t \]+print k.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendshr.c:$bp2_loc if x > 3.*" \
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp2_loc if x > 3.*" \
 "multiple pending breakpoints"
 
 
@@ -179,13 +179,13 @@ gdb_test {ignore $bpnum 2} "Will ignore next 2 crossings of breakpoint .*" \
     "set ignore count on pending breakpoint 3"
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep n\\(p\\).*PENDING.*pendfunc1.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
 \[\t \]+stop only if k == 1.*
 \[\t \]+print k.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendshr.c:$bp2_loc if x > 3.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*pendshr.c:$bp3_loc.*ignore next 2 hits.*" \
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp2_loc if x > 3.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp3_loc.*ignore next 2 hits.*" \
 "multiple pending breakpoints 2"
 
 #
@@ -253,7 +253,7 @@ gdb_test_multiple "break imaginary" "set imaginary pending breakpoint" {
 rerun_to_main
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*imaginary.*" \
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*imaginary.*" \
 "verify pending breakpoint after restart"
diff --git a/gdb/testsuite/gdb.base/sepdebug.exp b/gdb/testsuite/gdb.base/sepdebug.exp
index cd0d504..54534c6 100644
--- a/gdb/testsuite/gdb.base/sepdebug.exp
+++ b/gdb/testsuite/gdb.base/sepdebug.exp
@@ -179,7 +179,7 @@ set bp_location8 [gdb_get_line_number "set breakpoint 8 here"]
 set bp_location9 [gdb_get_line_number "set breakpoint 9 here"]
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$main_line.*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in marker2 at .*$srcfile:($bp_location8|$bp_location9).*
 \[0-9\]+\[\t \]+breakpoint     keep y.* in factorial at .*$srcfile:$bp_location7.*
@@ -298,7 +298,7 @@ gdb_test  "tbreak $srcfile:$bp_location11" "Breakpoint.*at.* file .*$srcfile, li
 #
 # check to see what breakpoints are set (temporary this time)
 #
-gdb_test "info break" "Num     Type.*Disp Enb  Address.*What.*\[\r\n\]
+gdb_test "info break" "Num     Type.*Disp Enb Address.*What.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in main at .*$srcfile:$main_line.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in factorial at .*$srcfile:$bp_location7.*\[\r\n\]
 \[0-9\]+\[\t \]+breakpoint     del.*y.*in main at .*$srcfile:$bp_location1.*\[\r\n\]
diff --git a/gdb/testsuite/gdb.base/unload.exp b/gdb/testsuite/gdb.base/unload.exp
index 55d5ff7..5ce1aaa 100644
--- a/gdb/testsuite/gdb.base/unload.exp
+++ b/gdb/testsuite/gdb.base/unload.exp
@@ -81,8 +81,8 @@ gdb_test_multiple "break shrfunc1" "set pending breakpoint" {
 }
 
 gdb_test "info break" \
-    "Num     Type\[ \]+Disp Enb  Address\[ \]+What.*
-\[0-9\]+\[\t \]+breakpoint     keep y\\(p\\).*PENDING.*shrfunc1.*" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*shrfunc1.*" \
 "single pending breakpoint info"
 
 set unloadshr_line [gdb_get_line_number "unloadshr break" ${libsrcfile}]
diff --git a/gdb/testsuite/gdb.cp/ovldbreak.exp b/gdb/testsuite/gdb.cp/ovldbreak.exp
index 7e52ce5..5f66c6e 100644
--- a/gdb/testsuite/gdb.cp/ovldbreak.exp
+++ b/gdb/testsuite/gdb.cp/ovldbreak.exp
@@ -154,7 +154,7 @@ set_bp_overloaded "foo::overload1arg" "$menu_overload1arg" 13   13 110
 # Verify the breakpoints.
 
 gdb_test "info break" \
-    "Num     Type\[\t \]+Disp Enb  Address\[\t \]+What.*
+    "Num     Type\[\t \]+Disp Enb Address\[\t \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in main at.*$srcfile:49\r
 \[\t \]+breakpoint already hit 1 time\r
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in foo::overload1arg\\(char\\) at.*$srcfile:111\r
@@ -212,7 +212,7 @@ gdb_expect {
 }
 
 gdb_test "info break" \
-    "Num     Type\[\t \]+Disp Enb  Address\[\t \]+What.*
+    "Num     Type\[\t \]+Disp Enb Address\[\t \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in main at.*$srcfile:49\r
 \[\t \]+breakpoint already hit 1 time\r
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in foo::overload1arg\\(char\\) at.*$srcfile:111\r
@@ -291,7 +291,7 @@ gdb_expect {
 }
 
 gdb_test "info break" \
-    "Num     Type\[\t \]+Disp Enb  Address\[\t \]+What.*
+    "Num     Type\[\t \]+Disp Enb Address\[\t \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in foo::overload1arg\\(double\\) at.*$srcfile:121\r
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in foo::overload1arg\\(float\\) at.*$srcfile:120\r
 \[0-9\]+\[\t \]+breakpoint     keep y\[\t \]+$hex\[\t \]+in foo::overload1arg\\(unsigned long\\) at.*$srcfile:119\r
diff --git a/gdb/testsuite/gdb.mi/mi-pending.exp b/gdb/testsuite/gdb.mi/mi-pending.exp
index ff74bcb..51c7f16 100644
--- a/gdb/testsuite/gdb.mi/mi-pending.exp
+++ b/gdb/testsuite/gdb.mi/mi-pending.exp
@@ -64,7 +64,7 @@ if [target_info exists gdb_stub] {
 
 # Set pending breakpoint via MI
 mi_gdb_test "-break-insert -f pendfunc1" \
-    ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\\(p\\)\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\"\}"\
+    ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\"\}"\
     "MI pending breakpoint on pendfunc1"
 
 mi_run_cmd
-- 
1.5.3.5


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-25 10:04                             ` Vladimir Prus
@ 2008-02-25 19:39                               ` Joel Brobecker
  2008-02-26 10:03                                 ` Vladimir Prus
  0 siblings, 1 reply; 25+ messages in thread
From: Joel Brobecker @ 2008-02-25 19:39 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: Nick Roberts, gdb-patches

Hi Volodya,

>     [gdb]
>     2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
> 
>             * breakpoint.c (print_one_breakpoint_location): Revert Enb field
>             to old format.  Discard breakpoint address if shared library is
>             unloaded.
>             (breakpoint_1): Adjust formatting of table header accordingly.
> 
>     [gdb/testsuite]
>     2008-02-25  Vladimir Prus  <vladimir@codesourcery.com>
> 
>         * gdb.base/annota1.exp: Adjust for 'info break'
>         format changes.
>         * gdb.base/annota3.exp: Likewise.
>         * gdb.base/break.exp: Likewise.
>         * gdb.base/condbreak.exp: Likewise.
>         * gdb.base/pending.exp: Likewise.
>         * gdb.base/sepdebug.exp: Likewise.
>         * gdb.base/unload.exp: Likewise.
>         * gdb.cp/ovldbreak.exp: Likewise.
>         * gdb.mi/mi-pending.exp: Likewise.

Pfew, some of these lines were so long that it was really tough to
review some of them, but after stretching my terminal to some 300
characters wide, it became much easier :-).

Patch is approved. Many thanks to Nick and Vladimir for getting
this done.


-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-25 19:39                               ` Joel Brobecker
@ 2008-02-26 10:03                                 ` Vladimir Prus
  2008-02-26 22:55                                   ` Joel Brobecker
  0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Prus @ 2008-02-26 10:03 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Nick Roberts, gdb-patches

On Monday 25 February 2008 22:29:41 Joel Brobecker wrote:
> Hi Volodya,
> 
> >     [gdb]
> >     2008-02-15  Nick Roberts  <nickrob@snap.net.nz>
> > 
> >             * breakpoint.c (print_one_breakpoint_location): Revert Enb field
> >             to old format.  Discard breakpoint address if shared library is
> >             unloaded.
> >             (breakpoint_1): Adjust formatting of table header accordingly.
> > 
> >     [gdb/testsuite]
> >     2008-02-25  Vladimir Prus  <vladimir@codesourcery.com>
> > 
> >         * gdb.base/annota1.exp: Adjust for 'info break'
> >         format changes.
> >         * gdb.base/annota3.exp: Likewise.
> >         * gdb.base/break.exp: Likewise.
> >         * gdb.base/condbreak.exp: Likewise.
> >         * gdb.base/pending.exp: Likewise.
> >         * gdb.base/sepdebug.exp: Likewise.
> >         * gdb.base/unload.exp: Likewise.
> >         * gdb.cp/ovldbreak.exp: Likewise.
> >         * gdb.mi/mi-pending.exp: Likewise.
> 
> Pfew, some of these lines were so long that it was really tough to
> review some of them, but after stretching my terminal to some 300
> characters wide, it became much easier :-).

Uhm, you mean those annota* tests? Those are scary indeed; I really wish
the annotations somehow disappear tomorrow.

> Patch is approved. Many thanks to Nick and Vladimir for getting
> this done.

Checked in.

- Volodya


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: (gdb-6.8) Discard breakpoint address if shared library is unloaded
  2008-02-26 10:03                                 ` Vladimir Prus
@ 2008-02-26 22:55                                   ` Joel Brobecker
  0 siblings, 0 replies; 25+ messages in thread
From: Joel Brobecker @ 2008-02-26 22:55 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: Nick Roberts, gdb-patches

> > Pfew, some of these lines were so long that it was really tough to
> > review some of them, but after stretching my terminal to some 300
> > characters wide, it became much easier :-).
> 
> Uhm, you mean those annota* tests? Those are scary indeed; I really wish
> the annotations somehow disappear tomorrow.

I wasn't really thinking about the annotations themselves, but rather
about the test where the expected output is one giant string 300-400
characters long, and the changes are 2 spaces that have been removed.
Verifying what has changed inside those lines was tricky... Maybe using
gdb_expect_list would improve the situation. But that's for another
rainy day...

-- 
Joel


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2008-02-26 22:37 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20080204214226.GF20922@adacore.com>
2008-02-04 21:55 ` (gdb-6.8) Discard breakpoint address if shared library is unloaded Nick Roberts
2008-02-05  0:12   ` Joel Brobecker
2008-02-05  0:21     ` Joel Brobecker
2008-02-05  0:36       ` Nick Roberts
2008-02-05  0:54     ` Nick Roberts
2008-02-07  6:38       ` Joel Brobecker
2008-02-08  1:37         ` Nick Roberts
2008-02-08  6:44           ` Vladimir Prus
2008-02-08  7:37             ` Nick Roberts
2008-02-14 21:43               ` Joel Brobecker
2008-02-15  4:01                 ` Nick Roberts
2008-02-15  8:39                   ` Eli Zaretskii
2008-02-15  9:13                     ` Nick Roberts
2008-02-16 12:59                       ` Eli Zaretskii
2008-02-17  9:56                   ` Vladimir Prus
2008-02-17 19:53                     ` Nick Roberts
2008-02-19 19:02                   ` Joel Brobecker
2008-02-19 20:04                     ` Nick Roberts
2008-02-20 16:31                       ` Joel Brobecker
2008-02-20 19:21                         ` Nick Roberts
2008-02-20 20:27                           ` Vladimir Prus
2008-02-25 10:04                             ` Vladimir Prus
2008-02-25 19:39                               ` Joel Brobecker
2008-02-26 10:03                                 ` Vladimir Prus
2008-02-26 22:55                                   ` Joel Brobecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox