pending breakpoints change breakpoint number

[Jim Ingham <jingham@apple.com>]

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

Hi, all...

I have been looking at the pending breakpoint changes (to merge them 
into the Apple gdb).  Overall it is nicer than the future-break version 
that we had, so that is great.

However, one thing really bugs me about it.  That is that it changes 
the breakpoint number out from under me when it manages to resolve the 
breakpoint.

This is pretty annoying to a human user, since if I want to do anything 
to the breakpoint, I have to go find it's number again from info break. 
  But it is very bad for a gui that is trying to run gdb from, say, the 
MI.  The GUI has to use the breakpoint number to do things like disable 
the breakpoint, delete it, etc.  So if the number switches out from 
under it, the gui can no longer operate on the breakpoint.  Very bad...

So there are two things that need to be addressed here.

First off, if we are going to change the breakpoint number, clearly 
need a breakpoint_resolved event/hook/observer thingie that we can use 
to tell the GUI that breakpoint 2 resolved to breakpoint 5, and the GUI 
needs to change accordingly.  This is pretty easy.  If it were a 
gdb_event, the addition to gdb-events.sh would look like:

2004-04-14  Jim Ingham  <jingham@apple.com>

         * gdb-events.sh: define a breakpoint_resolve event.
         * breakpoint.c (create_breakpoints): Post the event.
         * mi-main.h: Declare the mi resolve breakpoint hook.
         * mi-interp.c: Add a gdb_event structure.  Fill the 
breakpoint_resolve field.
         * (mi_interpreter_resume): Set the mi gdb_event handler.
         * mi-cmd-break.c (mi_async_breakpoint_resolve_event) New 
function.


[-- Attachment #2: pending-break.diff --]
[-- Type: application/octet-stream, Size: 4018 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.165
diff -p -p -r1.165 breakpoint.c
*** breakpoint.c	23 Mar 2004 14:47:55 -0000	1.165
--- breakpoint.c	16 Apr 2004 00:17:15 -0000
*************** create_breakpoints (struct symtabs_and_l
*** 4938,4943 ****
--- 4941,4949 ----
  	    if (pending_bp->commands)
  	      b->commands = copy_command_lines (pending_bp->commands);
  	  }
+         if (pending_bp != NULL)
+           breakpoint_resolve_event (pending_bp->number, b->number);
+ 
  	mention (b);
        }
    }    
Index: gdb-events.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdb-events.sh,v
retrieving revision 1.17
diff -p -p -r1.17 gdb-events.sh
*** gdb-events.sh       12 Feb 2004 00:17:53 -0000      1.17
--- gdb-events.sh       16 Apr 2004 00:20:40 -0000
*************** function_list ()
*** 61,66 ****
--- 61,67 ----
  f:void:breakpoint_create:int b:b
  f:void:breakpoint_delete:int b:b
  f:void:breakpoint_modify:int b:b
+ f:void:breakpoint_resolve:int b, int new_b:b, new_b
  f:void:tracepoint_create:int number:number
  f:void:tracepoint_delete:int number:number
  f:void:tracepoint_modify:int number:number
Index: mi/mi-cmd-break.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-break.c,v
retrieving revision 1.8
diff -p -p -r1.8 mi-cmd-break.c
*** mi/mi-cmd-break.c	19 Mar 2002 02:51:08 -0000	1.8
--- mi/mi-cmd-break.c	16 Apr 2004 00:17:18 -0000
***************
*** 21,26 ****
--- 21,27 ----
  
  #include "defs.h"
  #include "mi-cmds.h"
+ #include "mi-main.h"
  #include "ui-out.h"
  #include "mi-out.h"
  #include "breakpoint.h"
*************** mi_cmd_break_watch (char *command, char 
*** 238,240 ****
--- 239,260 ----
      }
    return MI_CMD_DONE;
  }
+ 
+ /* Report the resolution of a pended breakpoint.  */
+ void
+ mi_async_breakpoint_resolve_event (int b, int pending_b)
+ {
+   struct cleanup *old_chain;
+ 
+   if (pending_b > 0)
+     {
+       old_chain 
+ 	= make_cleanup_ui_out_notify_begin_end (uiout,
+ 						"resolve-pending-breakpoint");
+       ui_out_field_int (uiout, "new_bp", b);
+       ui_out_field_int (uiout, "pended_bp", pending_b);
+ 
+       do_cleanups (old_chain);
+     }
+ }
+ 
Index: mi/mi-interp.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v
retrieving revision 1.7
diff -p -p -r1.7 mi-interp.c
*** mi/mi-interp.c	19 Jan 2004 01:20:12 -0000	1.7
--- mi/mi-interp.c	16 Apr 2004 00:17:19 -0000
*************** struct mi_interp
*** 48,53 ****
--- 48,63 ----
    struct interp *mi_interp;
  };
  
+ /* These are the gdb events that we need to see
+    when we are running the the mi interpreter.  */
+ struct gdb_events mi_async_hooks =
+   {
+     NULL, /* breakpoint_create */
+     NULL, /* breakpoint_delete */
+     NULL, /* breakpoint_modify */
+     mi_async_breakpoint_resolve_event
+   };
+ 
  /* These are the interpreter setup, etc. functions for the MI interpreter */
  static void mi_execute_command_wrapper (char *cmd);
  static void mi_command_loop (int mi_version);
*************** mi_interpreter_resume (void *data)
*** 138,143 ****
--- 154,160 ----
      command_loop_hook = mi3_command_loop;
    else
      command_loop_hook = mi2_command_loop;
+   set_gdb_event_hooks (&mi_async_hooks);
  
    return 1;
  }
Index: mi/mi-main.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.h,v
retrieving revision 1.2
diff -p -p -r1.2 mi-main.h
*** mi/mi-main.h	6 Feb 2003 05:30:17 -0000	1.2
--- mi/mi-main.h	16 Apr 2004 00:17:19 -0000
*************** extern void mi_load_progress (const char
*** 29,33 ****
--- 29,34 ----
  			      unsigned long total_section,
  			      unsigned long total_sent,
  			      unsigned long grand_total);
+ void mi_async_breakpoint_resolve_event (int b, int pending_b);
  #endif
  

[-- Attachment #3: Type: text/plain, Size: 3458 bytes --]



I haven't played with the observer dingus that Andrew mentioned 
earlier, so I  don't know how it would look for that...

But even further, the question comes to mind "Why don't we just copy 
the old breakpoint's number over to the new breakpoint when it is 
made".  There are two objections to this I can think of.

One trivial one is that this leaves the breakpoint chain for a little 
while with two breakpoints with the same number.  I think it would be 
reasonable to declare that if you pass create_breakpoints a pending_bp, 
that breakpoint is create_breakpoint's to do with as it wishes - to 
delete if that is necessary or whatever.  Then we could delete it right 
when we made the new one and avoid this wart.  That might even be a 
little cleaner that having all the callers of 
resolve_pending_breakpoints have to delete the breakpoint.  OTOH, the 
callers all delete the pended breakpoint right away anyway, so this is 
more of a formal concern that a real problem...

A more substantial objection is that the pending breakpoint could end 
up resolving to more than one breakpoint.  Then you obviously can't 
just reuse the original number.

I think in the long term this is not actually correct.  As I understand 
it, the whole point of the breakpoint location structure within the 
breakpoint is to track these multiple breakpoint hits under one logical 
breakpoint.  So for something like a template or an inlined function, I 
would expect that we WOULD want there to be just one breakpoint, with 
multiple locations.  In that case, why not keep the breakpoint number?  
Moreover, in the vast majority of cases, the breakpoint goes one to one 
from the pended one to the new one.  So maybe in the short term, we 
could just detect whether there were only one or more than one sals, 
and reuse the number or not accordingly.  This would make it more 
convenient for casual use.

This is easy to do:

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.165
diff -p -p -r1.165 breakpoint.c
*** breakpoint.c        23 Mar 2004 14:47:55 -0000      1.165
--- breakpoint.c        16 Apr 2004 00:40:48 -0000
*************** create_breakpoints (struct symtabs_and_l
*** 4904,4911 ****
           describe_other_breakpoints (sal.pc, sal.section);

         b = set_raw_breakpoint (sal, type);
!       set_breakpoint_count (breakpoint_count + 1);
!       b->number = breakpoint_count;
         b->cond = cond[i];
         b->thread = thread;
         if (addr_string[i])
--- 4907,4921 ----
           describe_other_breakpoints (sal.pc, sal.section);

         b = set_raw_breakpoint (sal, type);
!         /* If we only made one breakpoint from a pending breakpoint,
!            don't change its number.  That's just annoying.  */
!         if (pending_bp != NULL && sals.nelts == 1)
!           b->number = pending_bp->number;
!         else
!           {
!             set_breakpoint_count (breakpoint_count + 1);
!             b->number = breakpoint_count;
!           }
         b->cond = cond[i];
         b->thread = thread;
         if (addr_string[i])

BTW, even if you don't make a new breakpoint number the resolved 
message is still useful, since the GUI might want to mark breakpoints 
differently if they are pended or have resolved.

Jim
--
Jim Ingham                                   jingham@apple.com
Developer Tools
Apple Computer