[RFA] breakpoints.c clear_command fix

[RFA] breakpoints.c clear_command fix

[Martin M. Hunt]

The clear command improperly detects overlays and fails 
to clear breakpoints if overlays are not disabled.

Tested with linux-x-mips (overlays enabled) and linux x86 native.
-- 
Martin Hunt
GDB Engineer
Red Hat, Inc.

2002-03-26  Martin M. Hunt  <hunt@redhat.com>

	* breakpoint.c (clear_command): Make the first loop 
	through breakpoints use the same conditions as the second.
	Correct both passes to properly detect when a breakpoint 
	is in an overlay.

Index: breakpoint.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/breakpoint.c,v
retrieving revision 1.331
diff -u -u -r1.331 breakpoint.c
--- breakpoint.c	2002/03/17 02:37:26	1.331
+++ breakpoint.c	2002/03/26 23:57:10
@@ -6445,24 +6445,20 @@
       sal = sals.sals[i];
       found = (struct breakpoint *) 0;
 
-
       while (breakpoint_chain
-      /* Why don't we check here that this is not
-         a watchpoint, etc., as we do below?
-         I can't make it fail, but don't know
-         what's stopping the failure: a watchpoint
-         of the same address as "sal.pc" should
-         wind up being deleted. */
-
-	     && (((sal.pc && (breakpoint_chain->address == sal.pc)) 
-		  && (!overlay_debugging 
-		      || breakpoint_chain->section == sal.section))
+	     && breakpoint_chain->type != bp_none
+	     && breakpoint_chain->type != bp_watchpoint
+	     && breakpoint_chain->type != bp_hardware_watchpoint
+	     && breakpoint_chain->type != bp_read_watchpoint
+	     && breakpoint_chain->type != bp_access_watchpoint
+	     && (((sal.pc && (breakpoint_chain->address == sal.pc))
+		  && (!section_is_overlay (breakpoint_chain->section)
+		      || section_is_mapped (breakpoint_chain->section)))
 		 || ((default_match || (0 == sal.pc))
 		     && breakpoint_chain->source_file != NULL
 		     && sal.symtab != NULL
-	      && STREQ (breakpoint_chain->source_file, sal.symtab->filename)
+		     && STREQ (breakpoint_chain->source_file, sal.symtab->filename)
 		     && breakpoint_chain->line_number == sal.line)))
-
 	{
 	  b1 = breakpoint_chain;
 	  breakpoint_chain = b1->next;
@@ -6478,14 +6474,13 @@
 	       && b->next->type != bp_read_watchpoint
 	       && b->next->type != bp_access_watchpoint
 	       && (((sal.pc && (b->next->address == sal.pc)) 
-		    && (!overlay_debugging || b->next->section == sal.section))
+		    && (!section_is_overlay (b->next->section)
+			|| section_is_mapped (b->next->section)))
 		   || ((default_match || (0 == sal.pc))
 		       && b->next->source_file != NULL
 		       && sal.symtab != NULL
 		       && STREQ (b->next->source_file, sal.symtab->filename)
 		       && b->next->line_number == sal.line)))
-
-
 	{
 	  b1 = b->next;
 	  b->next = b1->next;

Re: [RFA] breakpoints.c clear_command fix

[Michael Snyder]

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

"Martin M. Hunt" wrote:
> 
> The clear command improperly detects overlays and fails
> to clear breakpoints if overlays are not disabled.
> 
> Tested with linux-x-mips (overlays enabled) and linux x86 native.

Martin, you really made me think with this one.  Sorry it took so long.
I had to go back eleven years in the code base to understand what this
code was trying to do -- which made me realize that it's painfully
obsolete.  It has two inner loops with identical control conditions
(except that they've gotten out of sync), just because they didn't
have ALL_BREAKPOINTS_SAFE when this code was written.

So I rewrote the whole damn function.   ;-)

If you'd like to check the attached, to make sure that it 
preserves the intent of your change?  I take it your
intentions were:

  1) Make sure that the two inner loops have the same
     control conditions.  Instead of that, I combined them.
     Having two was just bad, it allowed them to get 
     out of sync in the first place.
  2) Allowing an overlay breakpoint to be cleared
     if overlay debugging is disabled.  I modified 
     that part of your change slightly.

[-- Attachment #2: clear.patch --]
[-- Type: text/plain, Size: 7192 bytes --]

2002-04-05  Michael Snyder  <msnyder@redhat.com>

	* breakpoint.c (clear_command): Rewrite middle section to
	combine two loops with identical control conditions.
	Add a cleanup to eliminate a memory leak.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.70
diff -p -r1.70 breakpoint.c
*** breakpoint.c	2002/04/05 19:16:15	1.70
--- breakpoint.c	2002/04/06 01:47:45
*************** tcatch_command (char *arg, int from_tty)
*** 6442,6456 ****
    catch_command_1 (arg, 1, from_tty);
  }
  
  
  static void
  clear_command (char *arg, int from_tty)
  {
!   register struct breakpoint *b, *b1;
    int default_match;
    struct symtabs_and_lines sals;
    struct symtab_and_line sal;
-   register struct breakpoint *found;
    int i;
  
    if (arg)
--- 6442,6456 ----
    catch_command_1 (arg, 1, from_tty);
  }
  
+ /* Delete breakpoints by address or line.  */
  
  static void
  clear_command (char *arg, int from_tty)
  {
!   struct breakpoint *b, *tmp, *prev, *found;
    int default_match;
    struct symtabs_and_lines sals;
    struct symtab_and_line sal;
    int i;
  
    if (arg)
*************** clear_command (char *arg, int from_tty)
*** 6462,6467 ****
--- 6462,6468 ----
      {
        sals.sals = (struct symtab_and_line *)
  	xmalloc (sizeof (struct symtab_and_line));
+       make_cleanup (xfree, sals.sals);
        INIT_SAL (&sal);		/* initialize to zeroes */
        sal.line = default_breakpoint_line;
        sal.symtab = default_breakpoint_symtab;
*************** clear_command (char *arg, int from_tty)
*** 6476,6488 ****
      }
  
    /* For each line spec given, delete bps which correspond
!      to it.  We do this in two loops: the first loop looks at
!      the initial bp(s) in the chain which should be deleted,
!      the second goes down the rest of the chain looking ahead
!      one so it can take those bps off the chain without messing
!      up the chain. */
  
! 
    for (i = 0; i < sals.nelts; i++)
      {
        /* If exact pc given, clear bpts at that pc.
--- 6477,6487 ----
      }
  
    /* For each line spec given, delete bps which correspond
!      to it.  Do it in two passes, solely to preserve the current
!      behavior that from_tty is forced true if we delete more than
!      one breakpoint.  */
  
!   found = NULL;
    for (i = 0; i < sals.nelts; i++)
      {
        /* If exact pc given, clear bpts at that pc.
*************** clear_command (char *arg, int from_tty)
*** 6498,6578 ****
           1              0             <can't happen> */
  
        sal = sals.sals[i];
!       found = (struct breakpoint *) 0;
! 
! 
!       while (breakpoint_chain
!       /* Why don't we check here that this is not
!          a watchpoint, etc., as we do below?
!          I can't make it fail, but don't know
!          what's stopping the failure: a watchpoint
!          of the same address as "sal.pc" should
!          wind up being deleted. */
! 
! 	     && (((sal.pc && (breakpoint_chain->address == sal.pc)) 
! 		  && (!overlay_debugging 
! 		      || breakpoint_chain->section == sal.section))
! 		 || ((default_match || (0 == sal.pc))
! 		     && breakpoint_chain->source_file != NULL
! 		     && sal.symtab != NULL
! 	      && STREQ (breakpoint_chain->source_file, sal.symtab->filename)
! 		     && breakpoint_chain->line_number == sal.line)))
! 
! 	{
! 	  b1 = breakpoint_chain;
! 	  breakpoint_chain = b1->next;
! 	  b1->next = found;
! 	  found = b1;
! 	}
! 
!       ALL_BREAKPOINTS (b)
! 	while (b->next
! 	       && b->next->type != bp_none
! 	       && b->next->type != bp_watchpoint
! 	       && b->next->type != bp_hardware_watchpoint
! 	       && b->next->type != bp_read_watchpoint
! 	       && b->next->type != bp_access_watchpoint
! 	       && (((sal.pc && (b->next->address == sal.pc)) 
! 		    && (!overlay_debugging || b->next->section == sal.section))
! 		   || ((default_match || (0 == sal.pc))
! 		       && b->next->source_file != NULL
! 		       && sal.symtab != NULL
! 		       && STREQ (b->next->source_file, sal.symtab->filename)
! 		       && b->next->line_number == sal.line)))
! 
! 
! 	{
! 	  b1 = b->next;
! 	  b->next = b1->next;
! 	  b1->next = found;
! 	  found = b1;
! 	}
  
!       if (found == 0)
  	{
! 	  if (arg)
! 	    error ("No breakpoint at %s.", arg);
  	  else
! 	    error ("No breakpoint at this line.");
  	}
  
!       if (found->next)
! 	from_tty = 1;		/* Always report if deleted more than one */
!       if (from_tty)
! 	printf_unfiltered ("Deleted breakpoint%s ", found->next ? "s" : "");
!       breakpoints_changed ();
!       while (found)
! 	{
! 	  if (from_tty)
! 	    printf_unfiltered ("%d ", found->number);
! 	  b1 = found->next;
! 	  delete_breakpoint (found);
! 	  found = b1;
! 	}
        if (from_tty)
! 	putchar_unfiltered ('\n');
      }
!   xfree (sals.sals);
  }
  \f
  /* Delete breakpoint in BS if they are `delete' breakpoints and
--- 6497,6571 ----
           1              0             <can't happen> */
  
        sal = sals.sals[i];
!       prev = NULL;
  
!       /* Find all matching breakpoints, remove them from the
! 	 breakpoint chain, and add them to the 'found' chain.  */
!       ALL_BREAKPOINTS_SAFE (b, tmp)
  	{
! 	  /* Are we going to delete b? */
! 	  if (b->type != bp_none
! 	      && b->type != bp_watchpoint
! 	      && b->type != bp_hardware_watchpoint
! 	      && b->type != bp_read_watchpoint
! 	      && b->type != bp_access_watchpoint
! 	      /* Not if b is a watchpoint of any sort... */
! 	      && (((sal.pc && (b->address == sal.pc)) 
! 		   && (!section_is_overlay (b->section)
! 		       || b->section == sal.section))
! 		  /* Yes, if sal.pc matches b (modulo overlays).  */
! 		  || ((default_match || (0 == sal.pc))
! 		      && b->source_file != NULL
! 		      && sal.symtab != NULL
! 		      && STREQ (b->source_file, sal.symtab->filename)
! 		      && b->line_number == sal.line)))
! 	    /* Yes, if sal source file and line matches b.  */
! 	    {
! 	      /* Remove it from breakpoint_chain...  */
! 	      if (b == breakpoint_chain)
! 		{
! 		  /* b is at the head of the list */
! 		  breakpoint_chain = b->next;
! 		}
! 	      else
! 		{
! 		  prev->next = b->next;
! 		}
! 	      /* And add it to 'found' chain.  */
! 	      b->next = found;
! 	      found = b;
! 	    }
  	  else
! 	    {
! 	      /* Keep b, and keep a pointer to it.  */
! 	      prev = b;
! 	    }
  	}
+     }
+   /* Now go thru the 'found' chain and delete them.  */
+   if (found == 0)
+     {
+       if (arg)
+ 	error ("No breakpoint at %s.", arg);
+       else
+ 	error ("No breakpoint at this line.");
+     }
  
!   if (found->next)
!     from_tty = 1;		/* Always report if deleted more than one */
!   if (from_tty)
!     printf_unfiltered ("Deleted breakpoint%s ", found->next ? "s" : "");
!   breakpoints_changed ();
!   while (found)
!     {
        if (from_tty)
! 	printf_unfiltered ("%d ", found->number);
!       tmp = found->next;
!       delete_breakpoint (found);
!       found = tmp;
      }
!   if (from_tty)
!     putchar_unfiltered ('\n');
  }
  \f
  /* Delete breakpoint in BS if they are `delete' breakpoints and

i18n; Was: [RFA] breakpoints.c clear_command fix

[Andrew Cagney]

>  if (from_tty)
> !     printf_unfiltered ("Deleted breakpoint%s ", found->next ? "s" : "");
> !

Completly off topic.  We're not ment to use this ``trick'' any more :-( 
  It doesn't work with i18n.  Instead, GDB is going to need to use:

	if (...)
	  print (singular)
	else
	  print (plural)

fun, eh,
Andrew

Re: i18n; Was: [RFA] breakpoints.c clear_command fix

[Eli Zaretskii]

> Date: Fri, 05 Apr 2002 21:40:19 -0500
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> >  if (from_tty)
> > !     printf_unfiltered ("Deleted breakpoint%s ", found->next ? "s" : "");
> > !
> 
> Completly off topic.  We're not ment to use this ``trick'' any more :-( 
>   It doesn't work with i18n.

That's true: this trick assumes English or something very similar.  It
doesn't work in languages that have several plural forms, not just 2,
and where the whole phrase might need non-trivial changes for
different numbers of breakpoints.

Btw, when (in what version) is it planned to add gettext support to
GDB?

Re: i18n; Was: [RFA] breakpoints.c clear_command fix

[Andrew Cagney]

> Date: Fri, 05 Apr 2002 21:40:19 -0500
>> From: Andrew Cagney <ac131313@cygnus.com>
>> 
> 
>> > if (from_tty)
>> > !     printf_unfiltered ("Deleted breakpoint%s ", found->next ? "s" : "");
>> > !
> 
>> 
>> Completly off topic.  We're not ment to use this ``trick'' any more :-( 
>> It doesn't work with i18n.
> 
> 
> That's true: this trick assumes English or something very similar.  It
> doesn't work in languages that have several plural forms, not just 2,
> and where the whole phrase might need non-trivial changes for
> different numbers of breakpoints.
> 
> Btw, when (in what version) is it planned to add gettext support to
> GDB?

I don't personally have plans to do this.  I've too many other things 
that I've started and not finished.

Andrew

Re: [RFA] breakpoints.c clear_command fix

[Martin M. Hunt]

On Friday 05 April 2002 05:55 pm, Michael Snyder wrote:
> "Martin M. Hunt" wrote:
> > The clear command improperly detects overlays and fails
> > to clear breakpoints if overlays are not disabled.
> >
> > Tested with linux-x-mips (overlays enabled) and linux x86 native.
>
> Martin, you really made me think with this one.  Sorry it took so long.
> I had to go back eleven years in the code base to understand what this
> code was trying to do -- which made me realize that it's painfully
> obsolete.  It has two inner loops with identical control conditions
> (except that they've gotten out of sync), just because they didn't
> have ALL_BREAKPOINTS_SAFE when this code was written.

I was wondering why the two seperate loops.  I didn't understand the comments 
so I made the minimum changes that seemed logical.

> So I rewrote the whole damn function.   ;-)

Great!

It looks fine and it passes all my test cases.  Thanks.

-- 
Martin Hunt
GDB Engineer
Red Hat, Inc.

Re: [RFA] breakpoints.c clear_command fix

[Michael Snyder]

"Martin M. Hunt" wrote:
> 
> On Friday 05 April 2002 05:55 pm, Michael Snyder wrote:
> > "Martin M. Hunt" wrote:
> > > The clear command improperly detects overlays and fails
> > > to clear breakpoints if overlays are not disabled.
> > >
> > > Tested with linux-x-mips (overlays enabled) and linux x86 native.
> >
> > Martin, you really made me think with this one.  Sorry it took so long.
> > I had to go back eleven years in the code base to understand what this
> > code was trying to do -- which made me realize that it's painfully
> > obsolete.  It has two inner loops with identical control conditions
> > (except that they've gotten out of sync), just because they didn't
> > have ALL_BREAKPOINTS_SAFE when this code was written.
> 
> I was wondering why the two seperate loops.  I didn't understand the comments
> so I made the minimum changes that seemed logical.
> 
> > So I rewrote the whole damn function.   ;-)
> 
> Great!
> 
> It looks fine and it passes all my test cases.  Thanks.

Committed.