[WIP] pending breakpoint support

[WIP] pending breakpoint support

[J. Johnston]

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

I have been hacking around with supporting "future-break" functionality (note, I 
said "hacking").  I have seen past discussion regarding the issue and the 
requirement to add functionality to the current breakpoint command as opposed to 
creating a separate command.

For my change, I wrappered the call to parse_breakpoint_sals() to catch any 
error.  If the call fails, then I simply give the user the option as marking it 
as pending.  I then make a fake breakpoint with a special enable_state of 
bp_shlib_pending.  I also save the original breakpoint command as the 
addr_string plus some needed state to recreate the command at a later time.  I 
attempted to have any original condition parsed but it isn't used in my current 
design as I end up reparsing the original command from scratch anyway.

When we are loading shared libraries, the function 
re_enable_breakpoints_in_shlibs() gets called.  I have added code in there to 
attempt to reparse any pending break command.  If successful, a new breakpoint 
is created by basically reissing the command with saved state accounted for. 
After creating the new breakpoint(s), I delete the pending place holder breakpoint.

Ideally, I would liked to have reused the same breakpoint structure that was 
initially allocated but in my code there still was the possibility of one 
command generating multiple breakpoints so I took the aforementioned strategy. 
I have been informed that the newer breakpoint model will eliminate that problem 
and I should be able to reuse the breakpoint number, etc...

A problem I didn't solve yet has to do with the issuing of error messages for 
pending and disabled shared-library breakpoints when the reenablement is 
attempted and it fails.  This can be very annoying when you have a large number 
of shared libraries loaded each time (e.g. Eclipse).

I have included a gdb session below along with the patch I used so folks can see 
how it currently works.

What I would like to do is get some discussion going:

   1. Is the user interface on the right track?
   2. What gotchas do I need to think about?
   3. Any design recommendations for implementing this better?

-- Jeff J.

/../gdb -nw a.out
GNU gdb 2003-11-07-cvs
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "ia64-unknown-linux-gnu"...Using host libthread_db 
library "/lib/tls/libthread_db.so.1".

(gdb) b printf
Function "printf" not defined.
Make breakpoint pending? (y or n) y
Breakpoint 1 (printf) pending.
(gdb) info break
Num Type           Disp Enb Address            What
1   breakpoint     keep n   <PENDING>          printf
(gdb) b main
Breakpoint 2 at 0x4000000000000702: file hello.c, line 9.
(gdb) info break
Num Type           Disp Enb Address            What
1   breakpoint     keep n   <PENDING>          printf
2   breakpoint     keep y   0x4000000000000702 in main at hello.c:9
(gdb) run
Starting program: 
/to/scratch/jjohnstn/gdb-patches/future-break/build/ia64/gdb/testsuite/gdb.base/a.out 

Pending breakpoint <printf> resolved
Breakpoint 3 at 0x20000000000ec5e0

Breakpoint 2, main () at hello.c:9
9	   x = printf ("hello world\n");
(gdb) info break
Num Type           Disp Enb Address            What
2   breakpoint     keep y   0x4000000000000702 in main at hello.c:9
	breakpoint already hit 1 time
3   breakpoint     keep y   0x20000000000ec5e0 <printf+48>
(gdb) c
Continuing.

Breakpoint 3, 0x20000000000ec5e0 in printf () from /lib/tls/libc.so.6.1
(gdb)

[-- Attachment #2: pending-break.patch --]
[-- Type: text/plain, Size: 13145 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.142
diff -u -p -r1.142 breakpoint.c
--- breakpoint.c	6 Nov 2003 18:35:05 -0000	1.142
+++ breakpoint.c	19 Nov 2003 00:51:08 -0000
@@ -119,6 +119,8 @@ static void condition_command (char *, i
 
 static int get_number_trailer (char **, int);
 
+static int do_captured_parse_breakpoint (void *);
+
 void set_breakpoint_count (int);
 
 typedef enum
@@ -2572,7 +2574,8 @@ bpstat_stop_status (CORE_ADDR *pc, int n
   {
     if (b->enable_state == bp_disabled
 	|| b->enable_state == bp_shlib_disabled
-	|| b->enable_state == bp_call_disabled)
+	|| b->enable_state == bp_call_disabled
+	|| b->enable_state == bp_shlib_pending)
       continue;
 
     if (b->type != bp_watchpoint
@@ -3300,7 +3303,7 @@ print_one_breakpoint (struct breakpoint 
   
   static char *bpdisps[] =
   {"del", "dstp", "dis", "keep"};
-  static char bpenables[] = "nynny";
+  static char bpenables[] = "nynnyn";
   char wrap_indent[80];
   struct ui_stream *stb = ui_out_stream_new (uiout);
   struct cleanup *old_chain = make_cleanup_ui_out_stream_delete (stb);
@@ -3454,7 +3457,15 @@ print_one_breakpoint (struct breakpoint 
 	if (addressprint)
 	  {
 	    annotate_field (4);
-	    ui_out_field_core_addr (uiout, "addr", b->loc->address);
+	    if (b->enable_state == bp_shlib_pending)
+	      {
+		if (TARGET_ADDR_BIT <= 32)
+		  ui_out_field_string (uiout, "addr", "<PENDING>  ");
+		else
+		  ui_out_field_string (uiout, "addr", "<PENDING>        ");
+	      }
+	    else
+	      ui_out_field_core_addr (uiout, "addr", b->loc->address);
 	  }
 	annotate_field (5);
 	*last_addr = b->loc->address;
@@ -3473,6 +3484,10 @@ print_one_breakpoint (struct breakpoint 
 	    ui_out_text (uiout, ":");
 	    ui_out_field_int (uiout, "line", b->line_number);
 	  }
+	else if (b->enable_state == bp_shlib_pending)
+	  {
+	    ui_out_field_string (uiout, "pending", b->addr_string);
+	  }
 	else
 	  {
 	    print_address_symbolic (b->loc->address, stb->stream, demangle, "");
@@ -3740,14 +3755,14 @@ describe_other_breakpoints (CORE_ADDR pc
 
   ALL_BREAKPOINTS (b)
     if (b->loc->address == pc)	/* address match / overlay match */
-      if (!overlay_debugging || b->loc->section == section)
+      if (b->enable_state != bp_shlib_pending && (!overlay_debugging || b->loc->section == section))
 	others++;
   if (others > 0)
     {
       printf_filtered ("Note: breakpoint%s ", (others > 1) ? "s" : "");
       ALL_BREAKPOINTS (b)
 	if (b->loc->address == pc)	/* address match / overlay match */
-	  if (!overlay_debugging || b->loc->section == section)
+	  if (b->enable_state != bp_shlib_pending && (!overlay_debugging || b->loc->section == section))
 	    {
 	      others--;
 	      printf_filtered ("%d%s%s ",
@@ -3836,6 +3851,7 @@ check_duplicates (struct breakpoint *bpt
   ALL_BP_LOCATIONS (b)
     if (b->owner->enable_state != bp_disabled
 	&& b->owner->enable_state != bp_shlib_disabled
+	&& b->owner->enable_state != bp_shlib_pending
 	&& b->owner->enable_state != bp_call_disabled
 	&& b->address == address	/* address / overlay match */
 	&& (!overlay_debugging || b->section == section)
@@ -3870,6 +3886,7 @@ check_duplicates (struct breakpoint *bpt
 	  {
 	    if (b->owner->enable_state != bp_disabled
 		&& b->owner->enable_state != bp_shlib_disabled
+		&& b->owner->enable_state != bp_shlib_pending
 		&& b->owner->enable_state != bp_call_disabled
 		&& b->address == address	/* address / overlay match */
 		&& (!overlay_debugging || b->section == section)
@@ -4284,22 +4301,84 @@ disable_breakpoints_in_shlibs (int silen
   }
 }
 
+struct captured_parse_breakpoint_args
+  {
+    char **arg_p;
+    struct symtabs_and_lines *sals_p;
+    char ***addr_string_p;
+  };
+
 /* Try to reenable any breakpoints in shared libraries.  */
 void
 re_enable_breakpoints_in_shlibs (void)
 {
   struct breakpoint *b;
+  struct breakpoint *del_b = NULL;
 
   ALL_BREAKPOINTS (b)
+  {
+    if (del_b)
+      {
+	delete_breakpoint (del_b);
+	del_b = NULL;
+	if (b == NULL)
+	  break;
+      }
     if (b->enable_state == bp_shlib_disabled)
-    {
-      char buf[1];
+      {
+	char buf[1];
+	
+	/* Do not reenable the breakpoint if the shared library
+	   is still not mapped in.  */
+	if (target_read_memory (b->loc->address, buf, 1) == 0)
+	  b->enable_state = bp_enabled;
+      }
+    else if (b->enable_state == bp_shlib_pending)
+      {
+	/* Try and reparse the breakpoint in case the shared library
+	   is now loaded.  */
+	struct symtabs_and_lines sals;
+	struct symtab_and_line pending_sal;
+	/* Pointers in arg to the start, and one past the end, of the
+	   condition.  */
+	char **cond_string = (char **) NULL;
+	char *copy_arg = b->addr_string;
+	char **addr_string;
+	struct captured_parse_breakpoint_args parse_args;
+	int rc;
+	
+	sals.sals = NULL;
+	sals.nelts = 0;
+	addr_string = NULL;
+	
+	parse_args.arg_p = &copy_arg;
+	parse_args.sals_p = &sals;
+	parse_args.addr_string_p = &addr_string;
+	
+	rc = catch_errors (do_captured_parse_breakpoint, &parse_args,
+			   NULL, RETURN_MASK_ALL);
 
-      /* Do not reenable the breakpoint if the shared library
-         is still not mapped in.  */
-      if (target_read_memory (b->loc->address, buf, 1) == 0)
-	b->enable_state = bp_enabled;
-    }
+	if (rc == GDB_RC_OK)
+	  {
+	    enum language old_language = current_language->la_language;
+	    int old_input_radix = input_radix;
+
+	    printf_filtered ("Pending breakpoint <%s> resolved\n", b->addr_string);
+
+	    /* Set language, input-radix, then reissue breakpoint command.  Following the
+	       command, restore the language and input-radix.  */
+	    set_language (b->language);
+	    input_radix = b->input_radix;
+	    break_command_1 (b->addr_string, b->flag, b->from_tty);
+	    set_language (old_language);
+	    input_radix = old_input_radix;
+	    del_b = b; /* Delete on next pass.  */
+	  }
+      }
+  }
+
+  if (del_b)
+    delete_breakpoint (del_b);
 }
 
 #endif
@@ -4712,14 +4791,21 @@ mention (struct breakpoint *b)
 
   if (say_where)
     {
-      if (addressprint || b->source_file == NULL)
+      if (b->enable_state == bp_shlib_pending)
 	{
-	  printf_filtered (" at ");
-	  print_address_numeric (b->loc->address, 1, gdb_stdout);
+	  printf_filtered (" (%s) pending.", b->addr_string);
+	}
+      else
+	{
+	  if (addressprint || b->source_file == NULL)
+	    {
+	      printf_filtered (" at ");
+	      print_address_numeric (b->loc->address, 1, gdb_stdout);
+	    }
+	  if (b->source_file)
+	    printf_filtered (": file %s, line %d.",
+			     b->source_file, b->line_number);
 	}
-      if (b->source_file)
-	printf_filtered (": file %s, line %d.",
-			 b->source_file, b->line_number);
     }
   do_cleanups (old_chain);
   if (ui_out_is_mi_like_p (uiout))
@@ -4892,6 +4978,16 @@ breakpoint_sals_to_pc (struct symtabs_an
     }
 }
 
+static int
+do_captured_parse_breakpoint (void *data)
+{
+  struct captured_parse_breakpoint_args *args = data;
+  
+  parse_breakpoint_sals (args->arg_p, args->sals_p, args->addr_string_p);
+
+  return GDB_RC_OK;
+}
+
 /* Set a breakpoint according to ARG (function, linenum or *address)
    flag: first bit  : 0 non-temporary, 1 temporary.
    second bit : 0 normal breakpoint, 1 hardware breakpoint. */
@@ -4902,14 +4998,18 @@ break_command_1 (char *arg, int flag, in
   int tempflag, hardwareflag;
   struct symtabs_and_lines sals;
   struct expression **cond = 0;
+  struct symtab_and_line pending_sal;
   /* Pointers in arg to the start, and one past the end, of the
      condition.  */
   char **cond_string = (char **) NULL;
+  char *copy_arg;
   char *addr_start = arg;
   char **addr_string;
   struct cleanup *old_chain;
   struct cleanup *breakpoint_chain = NULL;
-  int i;
+  struct captured_parse_breakpoint_args parse_args;
+  int i, rc;
+  int pending = 0;
   int thread = -1;
   int ignore_count = 0;
 
@@ -4919,19 +5019,40 @@ break_command_1 (char *arg, int flag, in
   sals.sals = NULL;
   sals.nelts = 0;
   addr_string = NULL;
-  parse_breakpoint_sals (&arg, &sals, &addr_string);
 
-  if (!sals.nelts)
+  parse_args.arg_p = &arg;
+  parse_args.sals_p = &sals;
+  parse_args.addr_string_p = &addr_string;
+
+  rc = catch_errors (do_captured_parse_breakpoint, &parse_args,
+		     NULL, RETURN_MASK_ALL);
+
+  if (rc != GDB_RC_OK)
+    {
+      if (!query ("Make breakpoint pending? "))
+	return;
+      copy_arg = (char *)xmalloc (strlen (addr_start));
+      strcpy (copy_arg, addr_start);
+      addr_string = &copy_arg;
+      sals.nelts = 1;
+      sals.sals = &pending_sal;
+      pending_sal.pc = 0;
+      pending = 1;
+    }
+  else if (!sals.nelts)
     return;
 
   /* Create a chain of things that always need to be cleaned up. */
   old_chain = make_cleanup (null_cleanup, 0);
 
-  /* Make sure that all storage allocated to SALS gets freed.  */
-  make_cleanup (xfree, sals.sals);
-
-  /* Cleanup the addr_string array but not its contents. */
-  make_cleanup (xfree, addr_string);
+  if (!pending)
+    {
+      /* Make sure that all storage allocated to SALS gets freed.  */
+      make_cleanup (xfree, sals.sals);
+      
+      /* Cleanup the addr_string array but not its contents. */
+      make_cleanup (xfree, addr_string);
+    }
 
   /* Allocate space for all the cond expressions. */
   cond = xcalloc (sals.nelts, sizeof (struct expression *));
@@ -4958,7 +5079,8 @@ break_command_1 (char *arg, int flag, in
 
   /* Resolve all line numbers to PC's and verify that the addresses
      are ok for the target.  */
-  breakpoint_sals_to_pc (&sals, addr_start);
+  if (!pending)
+    breakpoint_sals_to_pc (&sals, addr_start);
 
   /* Verify that condition can be parsed, before setting any
      breakpoints.  Allocate a separate condition expression for each
@@ -5009,11 +5131,34 @@ break_command_1 (char *arg, int flag, in
 	}
     }
 
-  create_breakpoints (sals, addr_string, cond, cond_string,
-		      hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
-		      tempflag ? disp_del : disp_donttouch,
-		      thread, ignore_count, from_tty);
+  if (pending)
+    {
+      struct symtab_and_line sal;
+      struct breakpoint *b;
+
+      sal.symtab = NULL;
+      sal.pc = 0;
 
+      b = set_raw_breakpoint (sal, hardwareflag ? bp_hardware_breakpoint : bp_breakpoint);
+      set_breakpoint_count (breakpoint_count + 1);
+      b->number = breakpoint_count;
+      b->cond = *cond;
+      b->thread = thread;
+      b->addr_string = *addr_string;
+      b->cond_string = *cond_string;
+      b->ignore_count = ignore_count;
+      b->enable_state = bp_shlib_pending;
+      b->disposition = tempflag ? disp_del : disp_donttouch;
+      b->from_tty = from_tty;
+      b->flag = flag;
+      mention (b);
+    }
+  else
+    create_breakpoints (sals, addr_string, cond, cond_string,
+			hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
+			tempflag ? disp_del : disp_donttouch,
+			thread, ignore_count, from_tty);
+  
   if (sals.nelts > 1)
     {
       warning ("Multiple breakpoints were set.");
@@ -6759,6 +6904,7 @@ delete_breakpoint (struct breakpoint *bp
 	    && !b->loc->duplicate
 	    && b->enable_state != bp_disabled
 	    && b->enable_state != bp_shlib_disabled
+	    && b->enable_state != bp_shlib_pending
 	    && b->enable_state != bp_call_disabled)
 	{
 	  int val;
@@ -6964,8 +7110,12 @@ breakpoint_re_set_one (void *bint)
          shlib_disabled breakpoint though.  There's a fair chance we
          can't re-set it if the shared library it's in hasn't been
          loaded yet.  */
+
+      if (b->enable_state == bp_shlib_pending)
+	break;
+
       save_enable = b->enable_state;
-      if (b->enable_state != bp_shlib_disabled)
+      if (b->enable_state != bp_shlib_disabled) 
         b->enable_state = bp_disabled;
 
       set_language (b->language);
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.26
diff -u -p -r1.26 breakpoint.h
--- breakpoint.h	6 Nov 2003 18:24:55 -0000	1.26
+++ breakpoint.h	19 Nov 2003 00:51:08 -0000
@@ -158,11 +158,14 @@ enum enable_state
 			   automatically enabled and reset when the call 
 			   "lands" (either completes, or stops at another 
 			   eventpoint). */
-    bp_permanent	/* There is a breakpoint instruction hard-wired into
+    bp_permanent,	/* There is a breakpoint instruction hard-wired into
 			   the target's code.  Don't try to write another
 			   breakpoint instruction on top of it, or restore
 			   its value.  Step over it using the architecture's
 			   SKIP_INSN macro.  */
+    bp_shlib_pending,   /* The eventpoint could not be set as an shlib has
+			   not yet been loaded the first time.  When the
+			   shlib is loaded, it will be reissued.  */
   };
 
 
@@ -385,6 +388,12 @@ struct breakpoint
 
     /* Methods associated with this breakpoint.  */
     struct breakpoint_ops *ops;
+
+    /* Initial from_tty value.  */
+    int from_tty;
+
+    /* Initial flag value.  */
+    int flag;
   };
 \f
 /* The following stuff is an abstract data type "bpstat" ("breakpoint

Re: [WIP] pending breakpoint support

[Eli Zaretskii]

> Date: Tue, 18 Nov 2003 20:03:09 -0500
> From: "J. Johnston" <jjohnstn@redhat.com>
> 
> What I would like to do is get some discussion going:
> 
>    1. Is the user interface on the right track?
>    2. What gotchas do I need to think about?
>    3. Any design recommendations for implementing this better?

I think you are on the right track, but please don't forget to include
a patch for the manual when this feature is about to go in.

Thanks.

Re: [WIP] pending breakpoint support

[Daniel Jacobowitz]

On Tue, Nov 18, 2003 at 08:03:09PM -0500, J. Johnston wrote:
> I have been hacking around with supporting "future-break" functionality 
> (note, I said "hacking").  I have seen past discussion regarding the issue 
> and the requirement to add functionality to the current breakpoint command 
> as opposed to creating a separate command.

Wonderful!  As you may guess, I have some comments :)

> For my change, I wrappered the call to parse_breakpoint_sals() to catch any 
> error.  If the call fails, then I simply give the user the option as 
> marking it as pending.  I then make a fake breakpoint with a special 
> enable_state of bp_shlib_pending.  I also save the original breakpoint 
> command as the addr_string plus some needed state to recreate the command 
> at a later time.  I attempted to have any original condition parsed but it 
> isn't used in my current design as I end up reparsing the original command 
> from scratch anyway.

I don't really like this implementation (bp_shlib_pending).  To explain
why, I need to take a step back to the last conversation about multiple
breakpoints.  Particularly, the question of what "break foo" should do
in various cases.  Here's some of the simple ones:

1.	int foo(void);
	int foo(int);

2.	int foo<int>(void);
	int foo<char>(void);

3.	foo.c:static int foo(void);
	bar.c:static int foo(void);

4.	libfoo.so:int foo(void);
	libbar.so:int foo(void);
	appbar.exe:int foo(void);

The answer for (1) is pretty straightforward.  Right now we prompt and
create multiple independent breakpoints, and I don't see a problem with
that.  I believe that's currently what we do for (2) also - a little
less clearcut but it still seems reasonable to me.

For (3) we currently pick one at random (whichever appears first); when
the global symbol lookup fails we iterate through static blocks. 
That's bad.  We should prompt, and do as in (1).

All clear so far, but the one I really wanted to point out was (4).  I
believe that it shouldn't be handled like the others.  Instead it
should be handled like I am planning to handle inlined functions - as
one breakpoint with multiple addresses (and some special handling for
PLT entries...).  For a particular example of where this is useful, on
at least Darwin the libsupc++ functions used for C++ exception handling
can appear in multiple shared objects, and each will use its own.  A
breakpoint on one of them had best be placed on all of them if you
really want GDB to catch all exceptions!  In general, in this case,
there are too many factors to predict which copy of a function will be
called; so the least confusing thing for GDB to do would be to
breakpoint on all of them.

This suggests that a different implementation is in order, because when
a shared library is loaded we may want to expand a bp_enabled
breakpoint to have two addresses also.  Support for breakpoints with a
variable number of location elements is coming "soon"; it's my next
planned project.

> When we are loading shared libraries, the function 
> re_enable_breakpoints_in_shlibs() gets called.  I have added code in there 
> to attempt to reparse any pending break command.  If successful, a new 
> breakpoint is created by basically reissing the command with saved state 
> accounted for. After creating the new breakpoint(s), I delete the pending 
> place holder breakpoint.

This raises an unpleasant question.  If one of the two functions above
doesn't have adequate debug information to evaluate the condition, what
do we do?  Punt?  What's punting in this case - making one of them
unconditional, making both of them unconditional, not breakpointing the
one?  Warning the user presumably.

One of many sticky interface problems I've thought of but not come up
with a good answer for.

> Ideally, I would liked to have reused the same breakpoint structure that 
> was initially allocated but in my code there still was the possibility of 
> one command generating multiple breakpoints so I took the aforementioned 
> strategy. I have been informed that the newer breakpoint model will 
> eliminate that problem and I should be able to reuse the breakpoint number, 
> etc...

Yes, this should be OK.

> A problem I didn't solve yet has to do with the issuing of error messages 
> for pending and disabled shared-library breakpoints when the reenablement 
> is attempted and it fails.  This can be very annoying when you have a large 
> number of shared libraries loaded each time (e.g. Eclipse).
> 
> I have included a gdb session below along with the patch I used so folks 
> can see how it currently works.
> 
> What I would like to do is get some discussion going:
> 
>   1. Is the user interface on the right track?
>   2. What gotchas do I need to think about?
>   3. Any design recommendations for implementing this better?

> (gdb) b printf
> Function "printf" not defined.
> Make breakpoint pending? (y or n) y
> Breakpoint 1 (printf) pending.

The wording needs to be improved.  If I hadn't been working on pending
breakpoints I'd have no idea what it meant.  How about something like
this (still very awkward):

(gb) b printf
Function "printf" not currently defined.
Create a pending breakpoint for later definitions of "printf" loaded
from shared libraries? (y or n) y
Breakpoint 1 (printf) pending.

> (gdb) info break
> Num Type           Disp Enb Address            What
> 1   breakpoint     keep n   <PENDING>          printf

One reason I'd like this separated from enable state; I think it should
be marked as y.  That way the user can "disable" it if necessary. 
Also, it would be nice to be symmetric with the bp_shlib_disabled
handling (similar problem).


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [WIP] pending breakpoint support

[Tom Tromey]

>>>>> "Jeff" == J Johnston <jjohnstn@redhat.com> writes:

Jeff> I have been hacking around with supporting "future-break"
Jeff> functionality (note, I said "hacking").

Jeff> If the call fails, then I simply give the user the option as
Jeff> marking it as pending.

This sounds great for interactive use.  For breakpoints set by IDEs,
it would be useful to have flag saying "please mark this as pending".

For instance, Insight saves all the breakpoints you set.  When opening
an old session, it tries to set them again programatically.  In this
case we already know the breakpoint is ok (or was last time around,
which is "close enough"), so we'd like not to ask the user anything.
There are similar scenarios for Emacs and Eclipse.

Tom

Re: [WIP] pending breakpoint support

[J. Johnston]

Daniel Jacobowitz wrote:
> On Tue, Nov 18, 2003 at 08:03:09PM -0500, J. Johnston wrote:
> 
>>I have been hacking around with supporting "future-break" functionality 
>>(note, I said "hacking").  I have seen past discussion regarding the issue 
>>and the requirement to add functionality to the current breakpoint command 
>>as opposed to creating a separate command.
> 
> 
> Wonderful!  As you may guess, I have some comments :)
> 
> 
>>For my change, I wrappered the call to parse_breakpoint_sals() to catch any 
>>error.  If the call fails, then I simply give the user the option as 
>>marking it as pending.  I then make a fake breakpoint with a special 
>>enable_state of bp_shlib_pending.  I also save the original breakpoint 
>>command as the addr_string plus some needed state to recreate the command 
>>at a later time.  I attempted to have any original condition parsed but it 
>>isn't used in my current design as I end up reparsing the original command 
>>from scratch anyway.
> 
> 
> I don't really like this implementation (bp_shlib_pending).  To explain
> why, I need to take a step back to the last conversation about multiple
> breakpoints.  Particularly, the question of what "break foo" should do
> in various cases.  Here's some of the simple ones:
> 
> 1.	int foo(void);
> 	int foo(int);
> 
> 2.	int foo<int>(void);
> 	int foo<char>(void);
> 
> 3.	foo.c:static int foo(void);
> 	bar.c:static int foo(void);
> 
> 4.	libfoo.so:int foo(void);
> 	libbar.so:int foo(void);
> 	appbar.exe:int foo(void);
> 
> The answer for (1) is pretty straightforward.  Right now we prompt and
> create multiple independent breakpoints, and I don't see a problem with
> that.  I believe that's currently what we do for (2) also - a little
> less clearcut but it still seems reasonable to me.
> 
> For (3) we currently pick one at random (whichever appears first); when
> the global symbol lookup fails we iterate through static blocks. 
> That's bad.  We should prompt, and do as in (1).
> 
> All clear so far, but the one I really wanted to point out was (4).  I
> believe that it shouldn't be handled like the others.  Instead it
> should be handled like I am planning to handle inlined functions - as
> one breakpoint with multiple addresses (and some special handling for
> PLT entries...).  For a particular example of where this is useful, on
> at least Darwin the libsupc++ functions used for C++ exception handling
> can appear in multiple shared objects, and each will use its own.  A
> breakpoint on one of them had best be placed on all of them if you
> really want GDB to catch all exceptions!  In general, in this case,
> there are too many factors to predict which copy of a function will be
> called; so the least confusing thing for GDB to do would be to
> breakpoint on all of them.
> 
> This suggests that a different implementation is in order, because when
> a shared library is loaded we may want to expand a bp_enabled
> breakpoint to have two addresses also.  Support for breakpoints with a
> variable number of location elements is coming "soon"; it's my next
> planned project.
> 

Well, we could treat the breakpoint as "try for all newly loaded shared 
libraries".  This way, the pending (or whatever you want to call it) breakpoint 
sticks around instead of being deleted after first resolution.  We could easily 
change it to be enabled/disabled simply by adding an additional enable_state 
(one means pending_enabled, the second means pending_disabled).  Alternatively, 
a new flag could be added.

To address Jim's concerns about having too many libraries to try the breakpoint 
on, a conditional could be added for the shared library or libraries to attempt 
the breakpoint on.  For example,

b printf
Function printf not found
Reattempt breakpoint later on shared library loads (Y/N)?
.
cond 1 $_shlib==libc.so.* || $_shlib == libm.so.*

In this case, the breakpoint would only be attempted if a shared library 
matching the templates libc.so.* or libm.so.* is being loaded.  I have 
overloaded the conditional as an example.  This could be done with a 
meta-conditional but I figure this might be simpler as a user can see everything 
at once with the current output.

One question would be what to do with the pending breakpoint when the program is 
rerun.  We could do a number of following options:

  1. remove it unconditionally (assume program has resolved and created all
     needed breakpoints)

  2. leave it around and check all resolved pending breakpoints, avoiding
     duplication (this would require a flag to indicate a breakpoint was a
     resolved pending breakpoint)

  3. put it under control of a new option

> 
>>When we are loading shared libraries, the function 
>>re_enable_breakpoints_in_shlibs() gets called.  I have added code in there 
>>to attempt to reparse any pending break command.  If successful, a new 
>>breakpoint is created by basically reissing the command with saved state 
>>accounted for. After creating the new breakpoint(s), I delete the pending 
>>place holder breakpoint.
> 
> 
> This raises an unpleasant question.  If one of the two functions above
> doesn't have adequate debug information to evaluate the condition, what
> do we do?  Punt?  What's punting in this case - making one of them
> unconditional, making both of them unconditional, not breakpointing the
> one?  Warning the user presumably.
> 

IMHO, it would be reasonable to issue a message regarding the conditional is not 
applicable for the shared library "x" and then offer the user the choice to 
offer an alternative condition (including empty condition) or to simply not set 
the breakpoint.

> One of many sticky interface problems I've thought of but not come up
> with a good answer for.
> 
> 
>>Ideally, I would liked to have reused the same breakpoint structure that 
>>was initially allocated but in my code there still was the possibility of 
>>one command generating multiple breakpoints so I took the aforementioned 
>>strategy. I have been informed that the newer breakpoint model will 
>>eliminate that problem and I should be able to reuse the breakpoint number, 
>>etc...
> 
> 
> Yes, this should be OK.
> 

Actually, by making the pending breakpoint stick around as I suggest above, I 
won't be deleting the breakpoint so this is no longer applicable.

> 
>>A problem I didn't solve yet has to do with the issuing of error messages 
>>for pending and disabled shared-library breakpoints when the reenablement 
>>is attempted and it fails.  This can be very annoying when you have a large 
>>number of shared libraries loaded each time (e.g. Eclipse).
>>
>>I have included a gdb session below along with the patch I used so folks 
>>can see how it currently works.
>>
>>What I would like to do is get some discussion going:
>>
>>  1. Is the user interface on the right track?
>>  2. What gotchas do I need to think about?
>>  3. Any design recommendations for implementing this better?
> 
> 
>>(gdb) b printf
>>Function "printf" not defined.
>>Make breakpoint pending? (y or n) y
>>Breakpoint 1 (printf) pending.
> 
> 
> The wording needs to be improved.  If I hadn't been working on pending
> breakpoints I'd have no idea what it meant.  How about something like
> this (still very awkward):
> 
> (gb) b printf
> Function "printf" not currently defined.
> Create a pending breakpoint for later definitions of "printf" loaded
> from shared libraries? (y or n) y
> Breakpoint 1 (printf) pending.
> 
> 
>>(gdb) info break
>>Num Type           Disp Enb Address            What
>>1   breakpoint     keep n   <PENDING>          printf
> 
> 
> One reason I'd like this separated from enable state; I think it should
> be marked as y.  That way the user can "disable" it if necessary. 
> Also, it would be nice to be symmetric with the bp_shlib_disabled
> handling (similar problem).
> 

I agree with the ability to enable/disable.  See above.
> 

Re: [WIP] pending breakpoint support

[Andrew Cagney]

> To address Jim's concerns about having too many libraries to try the breakpoint on, a conditional could be added for the shared library or libraries to attempt the breakpoint on.  For example,

I'd not spend much time on performance problems at this stage.  As they 
say get it working (simply and correctly) (the UI, test cases, ...); and 
then if time permits get it working fast.

Andrew

Re: [WIP] pending breakpoint support

[J. Johnston]

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

I have appended a patch to replace the previous hack.  I changed the code so it 
supports conditionals and doesn't fail if you specify source:line breakpoints. 
I also changed the mechanism to add a new flag to the breakpoint struct called 
"pending".  As you will notice, this adds a large number of checks because you 
can't just check for enable_state == bp_enabled without also checking for the 
pending flag.  I think that having two more enable states would have been 
simpler, but I will let all of you decide.  With this change, you can 
enable/disable the pending breakpoint and see any conditionals attached to it. 
Commands should also work.

I tried leaving the breakpoint around to allow for "break in all shared libs" 
but that experiment fell flat because it ended up creating the same breakpoint 
multiple times as each shared library gets loaded (e.g. it keeps finding printf 
over and over again in libc).  This patch still finds the first occurrence in a 
shared library and then deletes the pending breakpoint.  Are there plans to 
identify the shared library for a breakpoint in the proposed changes?

-- Jeff J.

J. Johnston wrote:
> Daniel Jacobowitz wrote:
> 
>> On Tue, Nov 18, 2003 at 08:03:09PM -0500, J. Johnston wrote:
>>
>>> I have been hacking around with supporting "future-break" 
>>> functionality (note, I said "hacking").  I have seen past discussion 
>>> regarding the issue and the requirement to add functionality to the 
>>> current breakpoint command as opposed to creating a separate command.
>>
>>
>>
>> Wonderful!  As you may guess, I have some comments :)
>>
>>
>>> For my change, I wrappered the call to parse_breakpoint_sals() to 
>>> catch any error.  If the call fails, then I simply give the user the 
>>> option as marking it as pending.  I then make a fake breakpoint with 
>>> a special enable_state of bp_shlib_pending.  I also save the original 
>>> breakpoint command as the addr_string plus some needed state to 
>>> recreate the command at a later time.  I attempted to have any 
>>> original condition parsed but it isn't used in my current design as I 
>>> end up reparsing the original command from scratch anyway.
>>
>>
>>
>> I don't really like this implementation (bp_shlib_pending).  To explain
>> why, I need to take a step back to the last conversation about multiple
>> breakpoints.  Particularly, the question of what "break foo" should do
>> in various cases.  Here's some of the simple ones:
>>
>> 1.    int foo(void);
>>     int foo(int);
>>
>> 2.    int foo<int>(void);
>>     int foo<char>(void);
>>
>> 3.    foo.c:static int foo(void);
>>     bar.c:static int foo(void);
>>
>> 4.    libfoo.so:int foo(void);
>>     libbar.so:int foo(void);
>>     appbar.exe:int foo(void);
>>
>> The answer for (1) is pretty straightforward.  Right now we prompt and
>> create multiple independent breakpoints, and I don't see a problem with
>> that.  I believe that's currently what we do for (2) also - a little
>> less clearcut but it still seems reasonable to me.
>>
>> For (3) we currently pick one at random (whichever appears first); when
>> the global symbol lookup fails we iterate through static blocks. 
>> That's bad.  We should prompt, and do as in (1).
>>
>> All clear so far, but the one I really wanted to point out was (4).  I
>> believe that it shouldn't be handled like the others.  Instead it
>> should be handled like I am planning to handle inlined functions - as
>> one breakpoint with multiple addresses (and some special handling for
>> PLT entries...).  For a particular example of where this is useful, on
>> at least Darwin the libsupc++ functions used for C++ exception handling
>> can appear in multiple shared objects, and each will use its own.  A
>> breakpoint on one of them had best be placed on all of them if you
>> really want GDB to catch all exceptions!  In general, in this case,
>> there are too many factors to predict which copy of a function will be
>> called; so the least confusing thing for GDB to do would be to
>> breakpoint on all of them.
>>
>> This suggests that a different implementation is in order, because when
>> a shared library is loaded we may want to expand a bp_enabled
>> breakpoint to have two addresses also.  Support for breakpoints with a
>> variable number of location elements is coming "soon"; it's my next
>> planned project.
>>
> 
> Well, we could treat the breakpoint as "try for all newly loaded shared 
> libraries".  This way, the pending (or whatever you want to call it) 
> breakpoint sticks around instead of being deleted after first 
> resolution.  We could easily change it to be enabled/disabled simply by 
> adding an additional enable_state (one means pending_enabled, the second 
> means pending_disabled).  Alternatively, a new flag could be added.
> 
> To address Jim's concerns about having too many libraries to try the 
> breakpoint on, a conditional could be added for the shared library or 
> libraries to attempt the breakpoint on.  For example,
> 
> b printf
> Function printf not found
> Reattempt breakpoint later on shared library loads (Y/N)?
> .
> cond 1 $_shlib==libc.so.* || $_shlib == libm.so.*
> 
> In this case, the breakpoint would only be attempted if a shared library 
> matching the templates libc.so.* or libm.so.* is being loaded.  I have 
> overloaded the conditional as an example.  This could be done with a 
> meta-conditional but I figure this might be simpler as a user can see 
> everything at once with the current output.
> 
> One question would be what to do with the pending breakpoint when the 
> program is rerun.  We could do a number of following options:
> 
>  1. remove it unconditionally (assume program has resolved and created all
>     needed breakpoints)
> 
>  2. leave it around and check all resolved pending breakpoints, avoiding
>     duplication (this would require a flag to indicate a breakpoint was a
>     resolved pending breakpoint)
> 
>  3. put it under control of a new option
> 
>>
>>> When we are loading shared libraries, the function 
>>> re_enable_breakpoints_in_shlibs() gets called.  I have added code in 
>>> there to attempt to reparse any pending break command.  If 
>>> successful, a new breakpoint is created by basically reissing the 
>>> command with saved state accounted for. After creating the new 
>>> breakpoint(s), I delete the pending place holder breakpoint.
>>
>>
>>
>> This raises an unpleasant question.  If one of the two functions above
>> doesn't have adequate debug information to evaluate the condition, what
>> do we do?  Punt?  What's punting in this case - making one of them
>> unconditional, making both of them unconditional, not breakpointing the
>> one?  Warning the user presumably.
>>
> 
> IMHO, it would be reasonable to issue a message regarding the 
> conditional is not applicable for the shared library "x" and then offer 
> the user the choice to offer an alternative condition (including empty 
> condition) or to simply not set the breakpoint.
> 
>> One of many sticky interface problems I've thought of but not come up
>> with a good answer for.
>>
>>
>>> Ideally, I would liked to have reused the same breakpoint structure 
>>> that was initially allocated but in my code there still was the 
>>> possibility of one command generating multiple breakpoints so I took 
>>> the aforementioned strategy. I have been informed that the newer 
>>> breakpoint model will eliminate that problem and I should be able to 
>>> reuse the breakpoint number, etc...
>>
>>
>>
>> Yes, this should be OK.
>>
> 
> Actually, by making the pending breakpoint stick around as I suggest 
> above, I won't be deleting the breakpoint so this is no longer applicable.
> 
>>
>>> A problem I didn't solve yet has to do with the issuing of error 
>>> messages for pending and disabled shared-library breakpoints when the 
>>> reenablement is attempted and it fails.  This can be very annoying 
>>> when you have a large number of shared libraries loaded each time 
>>> (e.g. Eclipse).
>>>
>>> I have included a gdb session below along with the patch I used so 
>>> folks can see how it currently works.
>>>
>>> What I would like to do is get some discussion going:
>>>
>>>  1. Is the user interface on the right track?
>>>  2. What gotchas do I need to think about?
>>>  3. Any design recommendations for implementing this better?
>>
>>
>>
>>> (gdb) b printf
>>> Function "printf" not defined.
>>> Make breakpoint pending? (y or n) y
>>> Breakpoint 1 (printf) pending.
>>
>>
>>
>> The wording needs to be improved.  If I hadn't been working on pending
>> breakpoints I'd have no idea what it meant.  How about something like
>> this (still very awkward):
>>
>> (gb) b printf
>> Function "printf" not currently defined.
>> Create a pending breakpoint for later definitions of "printf" loaded
>> from shared libraries? (y or n) y
>> Breakpoint 1 (printf) pending.
>>
>>
>>> (gdb) info break
>>> Num Type           Disp Enb Address            What
>>> 1   breakpoint     keep n   <PENDING>          printf
>>
>>
>>
>> One reason I'd like this separated from enable state; I think it should
>> be marked as y.  That way the user can "disable" it if necessary. 
>> Also, it would be nice to be symmetric with the bp_shlib_disabled
>> handling (similar problem).
>>
> 
> I agree with the ability to enable/disable.  See above.
> 
>>
> 
> 

[-- Attachment #2: pending-break.patch2 --]
[-- Type: text/plain, Size: 19873 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.142
diff -u -r1.142 breakpoint.c
--- breakpoint.c	6 Nov 2003 18:35:05 -0000	1.142
+++ breakpoint.c	21 Nov 2003 21:10:33 -0000
@@ -119,6 +119,8 @@
 
 static int get_number_trailer (char **, int);
 
+static int do_captured_parse_breakpoint (void *);
+
 void set_breakpoint_count (int);
 
 typedef enum
@@ -563,9 +565,12 @@
 	  /* I don't know if it matters whether this is the string the user
 	     typed in or the decompiled expression.  */
 	  b->cond_string = savestring (arg, strlen (arg));
-	  b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
-	  if (*arg)
-	    error ("Junk at end of expression");
+	  if (!b->pending)
+	    {
+	      b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+	      if (*arg)
+		error ("Junk at end of expression");
+	    }
 	}
       breakpoints_changed ();
       breakpoint_modify_event (b->number);
@@ -757,7 +762,7 @@
 
   /* Permanent breakpoints cannot be inserted or removed.  Disabled
      breakpoints should not be inserted.  */
-  if (bpt->owner->enable_state != bp_enabled)
+  if (bpt->owner->enable_state != bp_enabled || bpt->owner->pending)
     return 0;
 
   if (bpt->inserted || bpt->duplicate)
@@ -1103,7 +1108,7 @@
     {
       /* Permanent breakpoints cannot be inserted or removed.  Disabled
 	 breakpoints should not be inserted.  */
-      if (b->owner->enable_state != bp_enabled)
+      if (b->owner->enable_state != bp_enabled || b->owner->pending)
 	continue;
 
       /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
@@ -1454,6 +1459,7 @@
     }
   else if (b->loc_type == bp_loc_hardware_watchpoint
 	   && b->owner->enable_state == bp_enabled
+	   && !b->owner->pending
 	   && !b->duplicate)
     {
       struct value *v;
@@ -1510,6 +1516,7 @@
 	    b->owner->type == bp_catch_vfork ||
 	    b->owner->type == bp_catch_exec)
 	   && b->owner->enable_state == bp_enabled
+	   && !b->owner->pending
 	   && !b->duplicate)
     {
       val = -1;
@@ -1535,6 +1542,7 @@
   else if ((b->owner->type == bp_catch_catch ||
 	    b->owner->type == bp_catch_throw)
 	   && b->owner->enable_state == bp_enabled
+	   && !b->owner->pending
 	   && !b->duplicate)
     {
 
@@ -1546,6 +1554,7 @@
   else if (ep_is_exception_catchpoint (b->owner)
 	   && b->inserted	/* sometimes previous insert doesn't happen */
 	   && b->owner->enable_state == bp_enabled
+	   && !b->owner->pending
 	   && !b->duplicate)
     {
 
@@ -1671,7 +1680,8 @@
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if ((bpt->owner->enable_state == bp_enabled
+      if (((bpt->owner->enable_state == bp_enabled
+	   && !bpt->owner->pending)
 	   || bpt->owner->enable_state == bp_permanent)
 	  && bpt->address == pc)	/* bp is enabled and matches pc */
 	{
@@ -1768,7 +1778,8 @@
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if ((bpt->owner->enable_state == bp_enabled
+      if (((bpt->owner->enable_state == bp_enabled
+	   && !bpt->owner->pending)
 	   || bpt->owner->enable_state == bp_permanent)
 	  && bpt->address == pc
 	  && (bpt->owner->thread == -1 || bpt->owner->thread == thread))
@@ -2572,7 +2583,8 @@
   {
     if (b->enable_state == bp_disabled
 	|| b->enable_state == bp_shlib_disabled
-	|| b->enable_state == bp_call_disabled)
+	|| b->enable_state == bp_call_disabled
+	|| b->pending)
       continue;
 
     if (b->type != bp_watchpoint
@@ -3175,7 +3187,7 @@
 {
   struct breakpoint *b;
   ALL_BREAKPOINTS (b)
-    if (b->enable_state == bp_enabled && b->type == bp_watchpoint)
+    if (b->enable_state == bp_enabled && !b->pending && b->type == bp_watchpoint)
       return 1;
   return 0;
 }
@@ -3186,7 +3198,7 @@
 {
   struct bp_location *bpt;
   ALL_BP_LOCATIONS (bpt)
-    if ((bpt->owner->enable_state == bp_enabled)
+    if ((bpt->owner->enable_state == bp_enabled && !bpt->owner->pending)
 	&& bpt->inserted
 	&& bpt->loc_type == bp_loc_hardware_watchpoint)
       return 1;
@@ -3454,7 +3466,15 @@
 	if (addressprint)
 	  {
 	    annotate_field (4);
-	    ui_out_field_core_addr (uiout, "addr", b->loc->address);
+	    if (b->pending)
+	      {
+		if (TARGET_ADDR_BIT <= 32)
+		  ui_out_field_string (uiout, "addr", "<PENDING>  ");
+		else
+		  ui_out_field_string (uiout, "addr", "<PENDING>        ");
+	      }
+	    else
+	      ui_out_field_core_addr (uiout, "addr", b->loc->address);
 	  }
 	annotate_field (5);
 	*last_addr = b->loc->address;
@@ -3473,6 +3493,10 @@
 	    ui_out_text (uiout, ":");
 	    ui_out_field_int (uiout, "line", b->line_number);
 	  }
+	else if (b->pending)
+	  {
+	    ui_out_field_string (uiout, "pending", b->addr_string);
+	  }
 	else
 	  {
 	    print_address_symbolic (b->loc->address, stb->stream, demangle, "");
@@ -3509,7 +3533,15 @@
       ui_out_field_stream (uiout, "cond", stb);
       ui_out_text (uiout, "\n");
     }
-  
+
+  if (b->pending && b->cond_string)
+    {
+      annotate_field (7);
+      ui_out_text (uiout, "\tpending stop only if ");
+      ui_out_field_string (uiout, "cond", b->cond_string);
+      ui_out_text (uiout, "\n");
+    }
+
   if (b->thread != -1)
     {
       /* FIXME should make an annotation for this */
@@ -3740,14 +3772,14 @@
 
   ALL_BREAKPOINTS (b)
     if (b->loc->address == pc)	/* address match / overlay match */
-      if (!overlay_debugging || b->loc->section == section)
+      if (b->pending && (!overlay_debugging || b->loc->section == section))
 	others++;
   if (others > 0)
     {
       printf_filtered ("Note: breakpoint%s ", (others > 1) ? "s" : "");
       ALL_BREAKPOINTS (b)
 	if (b->loc->address == pc)	/* address match / overlay match */
-	  if (!overlay_debugging || b->loc->section == section)
+	  if (!b->pending && (!overlay_debugging || b->loc->section == section))
 	    {
 	      others--;
 	      printf_filtered ("%d%s%s ",
@@ -3836,6 +3868,7 @@
   ALL_BP_LOCATIONS (b)
     if (b->owner->enable_state != bp_disabled
 	&& b->owner->enable_state != bp_shlib_disabled
+	&& !b->owner->pending
 	&& b->owner->enable_state != bp_call_disabled
 	&& b->address == address	/* address / overlay match */
 	&& (!overlay_debugging || b->section == section)
@@ -3870,6 +3903,7 @@
 	  {
 	    if (b->owner->enable_state != bp_disabled
 		&& b->owner->enable_state != bp_shlib_disabled
+		&& !b->owner->pending
 		&& b->owner->enable_state != bp_call_disabled
 		&& b->address == address	/* address / overlay match */
 		&& (!overlay_debugging || b->section == section)
@@ -4046,6 +4080,7 @@
   b->forked_inferior_pid = 0;
   b->exec_pathname = NULL;
   b->ops = NULL;
+  b->pending = 0;
 
   /* Add this breakpoint to the end of the chain
      so that a list of breakpoints will come out in order
@@ -4265,6 +4300,7 @@
     if (((b->type == bp_breakpoint) ||
 	 (b->type == bp_hardware_breakpoint)) &&
 	b->enable_state == bp_enabled &&
+	!b->pending &&
 	!b->loc->duplicate &&
 	PC_SOLIB (b->loc->address))
       {
@@ -4284,22 +4320,104 @@
   }
 }
 
+struct captured_parse_breakpoint_args
+  {
+    char **arg_p;
+    struct symtabs_and_lines *sals_p;
+    char ***addr_string_p;
+  };
+
 /* Try to reenable any breakpoints in shared libraries.  */
 void
 re_enable_breakpoints_in_shlibs (void)
 {
   struct breakpoint *b;
+  struct breakpoint *del_b = NULL;
 
   ALL_BREAKPOINTS (b)
+  {
+    if (del_b)
+      {
+	delete_breakpoint (del_b);
+	del_b = NULL;
+	if (b == NULL)
+	  break;
+      }
     if (b->enable_state == bp_shlib_disabled)
-    {
-      char buf[1];
+      {
+	char buf[1];
+	
+	/* Do not reenable the breakpoint if the shared library
+	   is still not mapped in.  */
+	if (target_read_memory (b->loc->address, buf, 1) == 0)
+	  b->enable_state = bp_enabled;
+      }
+    else if (b->pending && (b->enable_state == bp_enabled))
+      {
+	/* Try and reparse the breakpoint in case the shared library
+	   is now loaded.  */
+	struct symtabs_and_lines sals;
+	struct symtab_and_line pending_sal;
+	/* Pointers in arg to the start, and one past the end, of the
+	   condition.  */
+	char **cond_string = (char **) NULL;
+	char *copy_arg = b->addr_string;
+	char **addr_string;
+	struct captured_parse_breakpoint_args parse_args;
+	int rc;
+	struct ui_file *old_gdb_stderr;
+	
+	sals.sals = NULL;
+	sals.nelts = 0;
+	addr_string = NULL;
+	
+	parse_args.arg_p = &copy_arg;
+	parse_args.sals_p = &sals;
+	parse_args.addr_string_p = &addr_string;
 
-      /* Do not reenable the breakpoint if the shared library
-         is still not mapped in.  */
-      if (target_read_memory (b->loc->address, buf, 1) == 0)
-	b->enable_state = bp_enabled;
-    }
+	old_gdb_stderr = gdb_stderr;
+	gdb_stderr = ui_file_new ();
+
+	rc = catch_errors (do_captured_parse_breakpoint, &parse_args,
+			   NULL, RETURN_MASK_ALL);
+
+	ui_file_delete (gdb_stderr);
+	gdb_stderr = old_gdb_stderr;
+
+	if (rc == GDB_RC_OK)
+	  {
+	    enum language old_language = current_language->la_language;
+	    int old_input_radix = input_radix;
+	    char *arg;
+	    struct breakpoint *b1;
+
+	    printf_filtered ("Pending breakpoint <%s> resolved\n", b->addr_string);
+
+	    /* Set language, input-radix, then reissue breakpoint command.  Following the
+	       command, restore the language and input-radix.  */
+	    set_language (b->language);
+	    input_radix = b->input_radix;
+	    break_command_1 (b->addr_string, b->flag, b->from_tty);
+	    b1 = breakpoint_chain;
+	    while (b1->next)
+	      b1 = b1->next;
+	    if (b->cond_string)
+	      {
+		arg = b->cond_string;
+		b1->cond_string = savestring (arg, strlen (arg));
+		b1->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+		if (*arg)
+		  error ("Junk at end of expression");
+	      }
+	    set_language (old_language);
+	    input_radix = old_input_radix;
+	    del_b = b;  /* Delete on next pass.  */
+	  }
+      }
+  }
+
+  if (del_b)
+    delete_breakpoint (del_b);
 }
 
 #endif
@@ -4486,7 +4604,7 @@
   *other_type_used = 0;
   ALL_BREAKPOINTS (b)
   {
-    if (b->enable_state == bp_enabled)
+    if (b->enable_state == bp_enabled && !b->pending)
       {
 	if (b->type == type)
 	  i++;
@@ -4712,14 +4830,21 @@
 
   if (say_where)
     {
-      if (addressprint || b->source_file == NULL)
+      if (b->pending)
 	{
-	  printf_filtered (" at ");
-	  print_address_numeric (b->loc->address, 1, gdb_stdout);
+	  printf_filtered (" (%s) pending.", b->addr_string);
+	}
+      else
+	{
+	  if (addressprint || b->source_file == NULL)
+	    {
+	      printf_filtered (" at ");
+	      print_address_numeric (b->loc->address, 1, gdb_stdout);
+	    }
+	  if (b->source_file)
+	    printf_filtered (": file %s, line %d.",
+			     b->source_file, b->line_number);
 	}
-      if (b->source_file)
-	printf_filtered (": file %s, line %d.",
-			 b->source_file, b->line_number);
     }
   do_cleanups (old_chain);
   if (ui_out_is_mi_like_p (uiout))
@@ -4892,6 +5017,16 @@
     }
 }
 
+static int
+do_captured_parse_breakpoint (void *data)
+{
+  struct captured_parse_breakpoint_args *args = data;
+  
+  parse_breakpoint_sals (args->arg_p, args->sals_p, args->addr_string_p);
+
+  return GDB_RC_OK;
+}
+
 /* Set a breakpoint according to ARG (function, linenum or *address)
    flag: first bit  : 0 non-temporary, 1 temporary.
    second bit : 0 normal breakpoint, 1 hardware breakpoint. */
@@ -4902,14 +5037,18 @@
   int tempflag, hardwareflag;
   struct symtabs_and_lines sals;
   struct expression **cond = 0;
+  struct symtab_and_line pending_sal;
   /* Pointers in arg to the start, and one past the end, of the
      condition.  */
   char **cond_string = (char **) NULL;
+  char *copy_arg;
   char *addr_start = arg;
   char **addr_string;
   struct cleanup *old_chain;
   struct cleanup *breakpoint_chain = NULL;
-  int i;
+  struct captured_parse_breakpoint_args parse_args;
+  int i, rc;
+  int pending = 0;
   int thread = -1;
   int ignore_count = 0;
 
@@ -4919,19 +5058,40 @@
   sals.sals = NULL;
   sals.nelts = 0;
   addr_string = NULL;
-  parse_breakpoint_sals (&arg, &sals, &addr_string);
 
-  if (!sals.nelts)
+  parse_args.arg_p = &arg;
+  parse_args.sals_p = &sals;
+  parse_args.addr_string_p = &addr_string;
+
+  rc = catch_errors (do_captured_parse_breakpoint, &parse_args,
+		     NULL, RETURN_MASK_ALL);
+
+  if (rc != GDB_RC_OK)
+    {
+      if (!query ("Make breakpoint pending? "))
+	return;
+      copy_arg = (char *)xmalloc (strlen (addr_start));
+      strcpy (copy_arg, addr_start);
+      addr_string = &copy_arg;
+      sals.nelts = 1;
+      sals.sals = &pending_sal;
+      pending_sal.pc = 0;
+      pending = 1;
+    }
+  else if (!sals.nelts)
     return;
 
   /* Create a chain of things that always need to be cleaned up. */
   old_chain = make_cleanup (null_cleanup, 0);
 
-  /* Make sure that all storage allocated to SALS gets freed.  */
-  make_cleanup (xfree, sals.sals);
-
-  /* Cleanup the addr_string array but not its contents. */
-  make_cleanup (xfree, addr_string);
+  if (!pending)
+    {
+      /* Make sure that all storage allocated to SALS gets freed.  */
+      make_cleanup (xfree, sals.sals);
+      
+      /* Cleanup the addr_string array but not its contents. */
+      make_cleanup (xfree, addr_string);
+    }
 
   /* Allocate space for all the cond expressions. */
   cond = xcalloc (sals.nelts, sizeof (struct expression *));
@@ -4958,62 +5118,87 @@
 
   /* Resolve all line numbers to PC's and verify that the addresses
      are ok for the target.  */
-  breakpoint_sals_to_pc (&sals, addr_start);
+  if (!pending)
+    breakpoint_sals_to_pc (&sals, addr_start);
 
   /* Verify that condition can be parsed, before setting any
      breakpoints.  Allocate a separate condition expression for each
      breakpoint. */
   thread = -1;			/* No specific thread yet */
-  for (i = 0; i < sals.nelts; i++)
+  if (!pending)
     {
-      char *tok = arg;
-      while (tok && *tok)
+      for (i = 0; i < sals.nelts; i++)
 	{
-	  char *end_tok;
-	  int toklen;
-	  char *cond_start = NULL;
-	  char *cond_end = NULL;
-	  while (*tok == ' ' || *tok == '\t')
-	    tok++;
-
-	  end_tok = tok;
-
-	  while (*end_tok != ' ' && *end_tok != '\t' && *end_tok != '\000')
-	    end_tok++;
-
-	  toklen = end_tok - tok;
-
-	  if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
+	  char *tok = arg;
+	  while (tok && *tok)
 	    {
-	      tok = cond_start = end_tok + 1;
-	      cond[i] = parse_exp_1 (&tok, block_for_pc (sals.sals[i].pc), 0);
-	      make_cleanup (xfree, cond[i]);
-	      cond_end = tok;
-	      cond_string[i] = savestring (cond_start, cond_end - cond_start);
-	      make_cleanup (xfree, cond_string[i]);
-	    }
-	  else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
-	    {
-	      char *tmptok;
-
-	      tok = end_tok + 1;
-	      tmptok = tok;
-	      thread = strtol (tok, &tok, 0);
-	      if (tok == tmptok)
-		error ("Junk after thread keyword.");
-	      if (!valid_thread_id (thread))
-		error ("Unknown thread %d\n", thread);
+	      char *end_tok;
+	      int toklen;
+	      char *cond_start = NULL;
+	      char *cond_end = NULL;
+	      while (*tok == ' ' || *tok == '\t')
+		tok++;
+	      
+	      end_tok = tok;
+	      
+	      while (*end_tok != ' ' && *end_tok != '\t' && *end_tok != '\000')
+		end_tok++;
+	      
+	      toklen = end_tok - tok;
+	      
+	      if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
+		{
+		  tok = cond_start = end_tok + 1;
+		  cond[i] = parse_exp_1 (&tok, block_for_pc (sals.sals[i].pc), 0);
+		  make_cleanup (xfree, cond[i]);
+		  cond_end = tok;
+		  cond_string[i] = savestring (cond_start, cond_end - cond_start);
+		  make_cleanup (xfree, cond_string[i]);
+		}
+	      else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+		{
+		  char *tmptok;
+		  
+		  tok = end_tok + 1;
+		  tmptok = tok;
+		  thread = strtol (tok, &tok, 0);
+		  if (tok == tmptok)
+		    error ("Junk after thread keyword.");
+		  if (!valid_thread_id (thread))
+		    error ("Unknown thread %d\n", thread);
+		}
+	      else
+		error ("Junk at end of arguments.");
 	    }
-	  else
-	    error ("Junk at end of arguments.");
 	}
+      create_breakpoints (sals, addr_string, cond, cond_string,
+			  hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
+			  tempflag ? disp_del : disp_donttouch,
+			  thread, ignore_count, from_tty);
     }
+  else
+    {
+      struct symtab_and_line sal;
+      struct breakpoint *b;
 
-  create_breakpoints (sals, addr_string, cond, cond_string,
-		      hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
-		      tempflag ? disp_del : disp_donttouch,
-		      thread, ignore_count, from_tty);
+      sal.symtab = NULL;
+      sal.pc = 0;
 
+      b = set_raw_breakpoint (sal, hardwareflag ? bp_hardware_breakpoint : bp_breakpoint);
+      set_breakpoint_count (breakpoint_count + 1);
+      b->number = breakpoint_count;
+      b->cond = *cond;
+      b->thread = thread;
+      b->addr_string = *addr_string;
+      b->cond_string = *cond_string;
+      b->ignore_count = ignore_count;
+      b->pending = 1;
+      b->disposition = tempflag ? disp_del : disp_donttouch;
+      b->from_tty = from_tty;
+      b->flag = flag;
+      mention (b);
+    }
+  
   if (sals.nelts > 1)
     {
       warning ("Multiple breakpoints were set.");
@@ -6759,6 +6944,7 @@
 	    && !b->loc->duplicate
 	    && b->enable_state != bp_disabled
 	    && b->enable_state != bp_shlib_disabled
+	    && !b->pending
 	    && b->enable_state != bp_call_disabled)
 	{
 	  int val;
@@ -6964,8 +7150,12 @@
          shlib_disabled breakpoint though.  There's a fair chance we
          can't re-set it if the shared library it's in hasn't been
          loaded yet.  */
+
+      if (b->pending)
+	break;
+
       save_enable = b->enable_state;
-      if (b->enable_state != bp_shlib_disabled)
+      if (b->enable_state != bp_shlib_disabled) 
         b->enable_state = bp_disabled;
 
       set_language (b->language);
@@ -7058,7 +7248,7 @@
 	value_free (b->val);
       b->val = evaluate_expression (b->exp);
       release_value (b->val);
-      if (VALUE_LAZY (b->val) && b->enable_state == bp_enabled)
+      if (VALUE_LAZY (b->val) && b->enable_state == bp_enabled && !b->pending)
 	value_fetch_lazy (b->val);
 
       if (b->cond_string != NULL)
@@ -7068,7 +7258,7 @@
 	    xfree (b->cond);
 	  b->cond = parse_exp_1 (&s, (struct block *) 0, 0);
 	}
-      if (b->enable_state == bp_enabled)
+      if (b->enable_state == bp_enabled && !b->pending)
 	mention (b);
       value_free_to_mark (mark);
       break;
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.26
diff -u -r1.26 breakpoint.h
--- breakpoint.h	6 Nov 2003 18:24:55 -0000	1.26
+++ breakpoint.h	21 Nov 2003 21:10:33 -0000
@@ -158,11 +158,14 @@
 			   automatically enabled and reset when the call 
 			   "lands" (either completes, or stops at another 
 			   eventpoint). */
-    bp_permanent	/* There is a breakpoint instruction hard-wired into
+    bp_permanent,	/* There is a breakpoint instruction hard-wired into
 			   the target's code.  Don't try to write another
 			   breakpoint instruction on top of it, or restore
 			   its value.  Step over it using the architecture's
 			   SKIP_INSN macro.  */
+    bp_shlib_pending,   /* The eventpoint could not be set as an shlib has
+			   not yet been loaded the first time.  When the
+			   shlib is loaded, it will be reissued.  */
   };
 
 
@@ -385,6 +388,15 @@
 
     /* Methods associated with this breakpoint.  */
     struct breakpoint_ops *ops;
+
+    /* Initial from_tty value.  */
+    int from_tty;
+
+    /* Initial flag value.  */
+    int flag;
+
+    /* Is breakpoint pending on shlib loads?  */
+    int pending;
   };
 \f
 /* The following stuff is an abstract data type "bpstat" ("breakpoint

Re: [WIP] pending breakpoint support

[Thomas Fitzsimmons]

On Fri, 21 Nov 2003 17:46:43 -0500, J. Johnston wrote:

> I have appended a patch to replace the previous hack.  I changed the
> code so it supports conditionals and doesn't fail if you specify
> source:line breakpoints. I also changed the mechanism to add a new flag
> to the breakpoint struct called "pending".  As you will notice, this
> adds a large number of checks because you can't just check for
> enable_state == bp_enabled without also checking for the pending flag. 
> I think that having two more enable states would have been simpler, but
> I will let all of you decide.  With this change, you can enable/disable
> the pending breakpoint and see any conditionals attached to it. Commands
> should also work.
> 

I've been using this patch since it was posted and it is very useful for
both Mozilla and libgcj debugging.  One minor UI nit: I don't think I
should be asked whether to make a breakpoint pending; I think the
"pending" message is enough (or maybe the behaviour could be
configurable).

In any case, even in its current early form, this patch is a huge
improvement for shared-library debugging.

Tom

Re: [WIP] pending breakpoint support

[Andrew Cagney]

> I tried leaving the breakpoint around to allow for "break in all shared libs" but that experiment fell flat because it ended up creating the same breakpoint multiple times as each shared library gets loaded (e.g. it keeps finding printf over and over again in libc).  This patch still finds the first occurrence in a shared library and then deletes the pending breakpoint.  Are there plans to identify the shared library for a breakpoint in the proposed changes?

That sounds reasonable for the first cut (who would do that anyway). 
Get the underlying mechanism working right, and then start to work out 
the human-computer-interface issues.  It's really part of that regex 
breakpoints, breakpoint groups type problem and can be addressed in a 
later pass - once we've got more experience with the problem.  File some 
bug reports :-)

Andrew

Re: [WIP] pending breakpoint support

[J. Johnston]

Thomas Fitzsimmons wrote:
> On Fri, 21 Nov 2003 17:46:43 -0500, J. Johnston wrote:
> 
> 
>>I have appended a patch to replace the previous hack.  I changed the
>>code so it supports conditionals and doesn't fail if you specify
>>source:line breakpoints. I also changed the mechanism to add a new flag
>>to the breakpoint struct called "pending".  As you will notice, this
>>adds a large number of checks because you can't just check for
>>enable_state == bp_enabled without also checking for the pending flag. 
>>I think that having two more enable states would have been simpler, but
>>I will let all of you decide.  With this change, you can enable/disable
>>the pending breakpoint and see any conditionals attached to it. Commands
>>should also work.
>>
> 
> 
> I've been using this patch since it was posted and it is very useful for
> both Mozilla and libgcj debugging.  One minor UI nit: I don't think I
> should be asked whether to make a breakpoint pending; I think the
> "pending" message is enough (or maybe the behaviour could be
> configurable).
>

IMO, the query should be the default.  If there is no query, every time you make 
a mistake, you would end up with an unwanted pending breakpoint.  Anybody, feel 
free to jump in with your comments.

Regarding the annoyance of having to answer many queries, this could be 
addressed in the future with a new setting, but that should be kept separate 
from the initial base implementation.

The query is not required for .gdbinit or mi.

-- Jeff J.

> In any case, even in its current early form, this patch is a huge
> improvement for shared-library debugging.
> 

Re: [WIP] pending breakpoint support

[Michael Snyder]

J. Johnston wrote:
> Thomas Fitzsimmons wrote:
> 
>> On Fri, 21 Nov 2003 17:46:43 -0500, J. Johnston wrote:
>>
>>
>>> I have appended a patch to replace the previous hack.  I changed the
>>> code so it supports conditionals and doesn't fail if you specify
>>> source:line breakpoints. I also changed the mechanism to add a new flag
>>> to the breakpoint struct called "pending".  As you will notice, this
>>> adds a large number of checks because you can't just check for
>>> enable_state == bp_enabled without also checking for the pending 
>>> flag. I think that having two more enable states would have been 
>>> simpler, but
>>> I will let all of you decide.  With this change, you can enable/disable
>>> the pending breakpoint and see any conditionals attached to it. Commands
>>> should also work.
>>>
>>
>>
>> I've been using this patch since it was posted and it is very useful for
>> both Mozilla and libgcj debugging.  One minor UI nit: I don't think I
>> should be asked whether to make a breakpoint pending; I think the
>> "pending" message is enough (or maybe the behaviour could be
>> configurable).
>>
> 
> IMO, the query should be the default.  If there is no query, every time 
> you make a mistake, you would end up with an unwanted pending 
> breakpoint.  Anybody, feel free to jump in with your comments.

That's right.  That's why at NeXT we had to keep some distinction
between a pending breakpoint and an ordinary breakpoint.  Otherwise,
you mistype "break mian", and you never find out you've made a mistake
until it's too late.

> Regarding the annoyance of having to answer many queries, this could be 
> addressed in the future with a new setting, but that should be kept 
> separate from the initial base implementation.

Maybe a "-f" (for force) option.  "Break here, and I really mean it".

Re: [WIP] pending breakpoint support

[Thomas Fitzsimmons]

On Wed, 2003-11-26 at 15:30, Michael Snyder wrote:
> J. Johnston wrote:
> > Thomas Fitzsimmons wrote:
> > 
> >> On Fri, 21 Nov 2003 17:46:43 -0500, J. Johnston wrote:
> >>
> >>
> >>> I have appended a patch to replace the previous hack.  I changed the
> >>> code so it supports conditionals and doesn't fail if you specify
> >>> source:line breakpoints. I also changed the mechanism to add a new flag
> >>> to the breakpoint struct called "pending".  As you will notice, this
> >>> adds a large number of checks because you can't just check for
> >>> enable_state == bp_enabled without also checking for the pending 
> >>> flag. I think that having two more enable states would have been 
> >>> simpler, but
> >>> I will let all of you decide.  With this change, you can enable/disable
> >>> the pending breakpoint and see any conditionals attached to it. Commands
> >>> should also work.
> >>>
> >>
> >>
> >> I've been using this patch since it was posted and it is very useful for
> >> both Mozilla and libgcj debugging.  One minor UI nit: I don't think I
> >> should be asked whether to make a breakpoint pending; I think the
> >> "pending" message is enough (or maybe the behaviour could be
> >> configurable).
> >>
> > 
> > IMO, the query should be the default.  If there is no query, every time 
> > you make a mistake, you would end up with an unwanted pending 
> > breakpoint.  Anybody, feel free to jump in with your comments.
> 
> That's right.  That's why at NeXT we had to keep some distinction
> between a pending breakpoint and an ordinary breakpoint.  Otherwise,
> you mistype "break mian", and you never find out you've made a mistake
> until it's too late.
> 

The differences in the pending and non-pending output messages should be
enough to indicate that something's wrong:

Breakpoint 1 (mian) pending.

vs.

Breakpoint 1 at 0x806b513: file main.c, line 180.

Anyway, it's not a big deal.

> > Regarding the annoyance of having to answer many queries, this could be 
> > addressed in the future with a new setting, but that should be kept 
> > separate from the initial base implementation.
> 
> Maybe a "-f" (for force) option.  "Break here, and I really mean it".
> 

Hmm, I think I would prefer a setting.  But like Jeff said, that feature
can be added later if it's generally desired.

Tom

Re: [WIP] pending breakpoint support

[Daniel Jacobowitz]

On Fri, Nov 21, 2003 at 05:46:43PM -0500, J. Johnston wrote:
> I tried leaving the breakpoint around to allow for "break in all shared 
> libs" but that experiment fell flat because it ended up creating the same 
> breakpoint multiple times as each shared library gets loaded (e.g. it keeps 
> finding printf over and over again in libc).  This patch still finds the 
> first occurrence in a shared library and then deletes the pending 
> breakpoint.  Are there plans to identify the shared library for a 
> breakpoint in the proposed changes?

No concrete plans, but this definitely can and should be addressed
later.  I have a couple of ideas but I want to get something
implemented (eventually) before speculating.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [WIP] pending breakpoint support

[Jim Ingham]

On Nov 19, 2003, at 8:27 AM, gdb-patches-digest-help@sources.redhat.com 
wrote:

> On Tue, Nov 18, 2003 at 08:03:09PM -0500, J. Johnston wrote:
>> I have been hacking around with supporting "future-break" 
>> functionality
>> (note, I said "hacking").  I have seen past discussion regarding the 
>> issue
>> and the requirement to add functionality to the current breakpoint 
>> command
>> as opposed to creating a separate command.
>
> Wonderful!  As you may guess, I have some comments :)

I concur!  Thanks for doing this!

>
>> For my change, I wrappered the call to parse_breakpoint_sals() to 
>> catch any
>> error.  If the call fails, then I simply give the user the option as
>> marking it as pending.  I then make a fake breakpoint with a special
>> enable_state of bp_shlib_pending.  I also save the original breakpoint
>> command as the addr_string plus some needed state to recreate the 
>> command
>> at a later time.  I attempted to have any original condition parsed 
>> but it
>> isn't used in my current design as I end up reparsing the original 
>> command
>> from scratch anyway.
>
> I don't really like this implementation (bp_shlib_pending).  To explain
> why, I need to take a step back to the last conversation about multiple
> breakpoints.  Particularly, the question of what "break foo" should do
> in various cases.  Here's some of the simple ones:
>
> 1.	int foo(void);
> 	int foo(int);
>
> 2.	int foo<int>(void);
> 	int foo<char>(void);
>
> 3.	foo.c:static int foo(void);
> 	bar.c:static int foo(void);
>
> 4.	libfoo.so:int foo(void);
> 	libbar.so:int foo(void);
> 	appbar.exe:int foo(void);
>
> The answer for (1) is pretty straightforward.  Right now we prompt and
> create multiple independent breakpoints, and I don't see a problem with
> that.  I believe that's currently what we do for (2) also - a little
> less clearcut but it still seems reasonable to me.
>
> For (3) we currently pick one at random (whichever appears first); when
> the global symbol lookup fails we iterate through static blocks.
> That's bad.  We should prompt, and do as in (1).
>
> All clear so far, but the one I really wanted to point out was (4).  I
> believe that it shouldn't be handled like the others.  Instead it
> should be handled like I am planning to handle inlined functions - as
> one breakpoint with multiple addresses (and some special handling for
> PLT entries...).  For a particular example of where this is useful, on
> at least Darwin the libsupc++ functions used for C++ exception handling
> can appear in multiple shared objects, and each will use its own.  A
> breakpoint on one of them had best be placed on all of them if you
> really want GDB to catch all exceptions!  In general, in this case,
> there are too many factors to predict which copy of a function will be
> called; so the least confusing thing for GDB to do would be to
> breakpoint on all of them.

There are two different needs here.  The case of the exception handling 
is one extreme, where we not only want to set all the breakpoints we 
can currently find, but keep watching for any future chance to set the 
breakpoint.  The other extreme is when you have an IDE that KNOWS that 
this breakpoint is being set in a project that produces a given shared 
library or executable.  In that case, you want to specify only a 
particular shared library.

On MacOS X, I also have some cases where there is enough shared library 
activity that I don't actually want to be re-evaluating ALL the 
breakpoints every time I get a load event.  Our case is not so common - 
it mostly arises from the ZeroLink style of building applications where 
all the .o's get made into little shlib's instead of being linked 
together (this is done to speed up turn-around time) and so you can get 
~500 or so shlibs loading, in batches of ~10 at a time, in the startup 
of a medium sized app.  Still, it is worth keeping in mind...

This need also arises for #4 when the IDE holds breakpoints 
persistently across runs.  In that case, the sensible thing to do is 
tell the user you found multiple instances, and ask which ones they 
want.  But then when you rerun the debugger, you really don't want to 
ask again.  You need instead to record enough info, and be able to pass 
it to gdb, to reset the breakpoint in the right shlib.  This is 
actually pretty easy to do in gdb - I hacked it into our version for 
the mi.  But when we discussed this last time we didn't come to any 
consensus about how to pass this through the cli break command, so I 
didn't do it there...


>
> This suggests that a different implementation is in order, because when
> a shared library is loaded we may want to expand a bp_enabled
> breakpoint to have two addresses also.  Support for breakpoints with a
> variable number of location elements is coming "soon"; it's my next
> planned project.


This is great!  One thing that we need to be sure is that we don't lock 
ourselves into an all or nothing solution here either.  It should be 
possible to say "I am happy with this breakpoint, don't keep looking" 
as well as to keep looking.

>
>> When we are loading shared libraries, the function
>> re_enable_breakpoints_in_shlibs() gets called.  I have added code in 
>> there
>> to attempt to reparse any pending break command.  If successful, a new
>> breakpoint is created by basically reissing the command with saved 
>> state
>> accounted for. After creating the new breakpoint(s), I delete the 
>> pending
>> place holder breakpoint.
>
> This raises an unpleasant question.  If one of the two functions above
> doesn't have adequate debug information to evaluate the condition, what
> do we do?  Punt?  What's punting in this case - making one of them
> unconditional, making both of them unconditional, not breakpointing the
> one?  Warning the user presumably.
>
> One of many sticky interface problems I've thought of but not come up
> with a good answer for.

Did you move the cond field of the breakpoint part out of the umbrella 
breakpoint structure in your patch, I don't remember.  That will be 
necessary here because even if the condition is parseable  in two 
contexts the struct expression that results is different.

>
>> Ideally, I would liked to have reused the same breakpoint structure 
>> that
>> was initially allocated but in my code there still was the 
>> possibility of
>> one command generating multiple breakpoints so I took the 
>> aforementioned
>> strategy. I have been informed that the newer breakpoint model will
>> eliminate that problem and I should be able to reuse the breakpoint 
>> number,
>> etc...
>
> Yes, this should be OK.
>
>> A problem I didn't solve yet has to do with the issuing of error 
>> messages
>> for pending and disabled shared-library breakpoints when the 
>> reenablement
>> is attempted and it fails.  This can be very annoying when you have a 
>> large
>> number of shared libraries loaded each time (e.g. Eclipse).
>>
>> I have included a gdb session below along with the patch I used so 
>> folks
>> can see how it currently works.
>>
>> What I would like to do is get some discussion going:
>>
>>   1. Is the user interface on the right track?
>>   2. What gotchas do I need to think about?
>>   3. Any design recommendations for implementing this better?
>
>> (gdb) b printf
>> Function "printf" not defined.
>> Make breakpoint pending? (y or n) y
>> Breakpoint 1 (printf) pending.
>
> The wording needs to be improved.  If I hadn't been working on pending
> breakpoints I'd have no idea what it meant.  How about something like
> this (still very awkward):
>
> (gb) b printf
> Function "printf" not currently defined.
> Create a pending breakpoint for later definitions of "printf" loaded
> from shared libraries? (y or n) y
> Breakpoint 1 (printf) pending.

Note that an IDE (presumably through the mi) will most likely want to 
be able to always answer "yes" to this without having to answer back. 
That will work in the code that you currently have, but only because 
query answers yes when you are not a tty.  This seems to me a fragile 
form of control here.  It would be better if there were a way for the 
mi command to explicitly override the question with a yes or no answer.

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