Re: [RFA] breakpoints.c clear_command fix

[Michael Snyder <msnyder@redhat.com>]

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