pending breakpoints change breakpoint number

pending breakpoints change breakpoint number

[Jim Ingham]

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

Re: pending breakpoints change breakpoint number

[Andrew Cagney]

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

The breakpoint number changing is a known bug in the current 
implementation.  It was concluded that fixing it (a non-trivial task) 
was closely tied to fixing things like M:N breakpoints (one breakpoint 
with multiple locations) and hence should be made part of that task 
(painful but working pending breakpoints were considered better than no 
pending breakpoints).

Seems that the N:M breakpoints have fallen by the wayside.  Interested 
in working on that?  Daniel can give you a status report of where things 
are at.

Andrew

Re: pending breakpoints change breakpoint number

[Jim Ingham]

Andrew,

Yes, the one symbolic->many actual breakpoints is something I am 
interested in working on.  It is high on my "list of tasks I keep 
trying to get into the next release".  Sadly, it didn't make it into 
the current release, so it will be a little while (certainly not till 
after WWDC in July) before I can get to this.

I still think the notification is necessary, and is a fairly small 
change.  I am sure the Eclipse folks are also going to run into this 
like the Xcode folks did.  And if we also include the address(s) the 
breakpoint resolved to (and maybe the symbol name - which would be 
useful for file:line breakpoints in template functions), then the 
notification will be useful even after we fix the M:N problem.  It 
means that a GUI which displays the addresses of breakpoints as part of 
its breakpoint display can keep these up to date without needing an 
extra round trip to gdb...

I fixed it in our sources so that I only change the breakpoint number 
when we set more than one breakpoint.  That was a simple change since 
you already have the pending breakpoint in create_breakpoints, though 
it is admittedly a bit of a hack.  Still it actually fixes most cases 
and seems to me better than the current state of things.  But this 
isn't nearly as important as the notification.

Jim

On May 5, 2004, at 7:52 AM, Andrew Cagney wrote:

>> 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:
>
> The breakpoint number changing is a known bug in the current 
> implementation.  It was concluded that fixing it (a non-trivial task) 
> was closely tied to fixing things like M:N breakpoints (one breakpoint 
> with multiple locations) and hence should be made part of that task 
> (painful but working pending breakpoints were considered better than 
> no pending breakpoints).
>
> Seems that the N:M breakpoints have fallen by the wayside.  Interested 
> in working on that?  Daniel can give you a status report of where 
> things are at.
>
> Andrew
>
>
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools
Apple Computer