Re: [4/4] RFC: implement catch load and catch unload

[Pedro Alves <palves at redhat dot com>]

Handling this piecemeal.

On 01/19/2012 08:35 PM, Tom Tromey wrote:

> * There is a change in internal_bkpt_check_status that is somewhat
>   hacky.  This was the simplest way I could find to make it so that
>   "catch load" reported the catchpoint; without this, bpstat_print would
>   find the internal bp_shlib_event bpstat and print it instead, even if
>   stop-on-solib-events was not set.

Hmm.  Should we ever print if the breakpoint does not cause a stop?  IOW,
can't we make that unconditional if we're also clearing bs->stop?

Related, it looks like there's a bug here:

bpstat_stop_status:

      if (bs->stop)
	{
...
	  /* Print nothing for this entry if we don't stop or don't print.  */
	  if (bs->stop == 0 || bs->print == 0)
	    bs->print_it = print_it_noop;
	}

That "Print nothing" only triggers if we think to stop, and then the
conditions evaluate false, or the breakpoint is silent.  That should
probably be moved out of the `if (bs->stop) condition' above.

Having said that, I've put it in patch form, on top of mainline.  WDYT?

	* breakpoint.c (bpstat_stop_status): Moving clearing print_it
	outside `bs->stop' block.
	(bpstat_what): Rework bp_shlib_event handling.
	(internal_bkpt_check_status): If the breakpoint is a
	bp_shlib_event, then set bs->stop and bs->print if
	stop_on_solib_events is set.
---

 gdb/breakpoint.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f6a0276..3f3a417 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4257,10 +4257,12 @@ bpstat_stop_status (struct address_space *aspace,
 		bs->print = 0;
 	    }

-	  /* Print nothing for this entry if we don't stop or don't print.  */
-	  if (bs->stop == 0 || bs->print == 0)
-	    bs->print_it = print_it_noop;
 	}
+
+      /* Print nothing for this entry if we don't stop or don't
+	 print.  */
+      if (!bs->stop || !bs->print)
+	bs->print_it = print_it_noop;
     }

   /* If we aren't stopping, the value of some hardware watchpoint may
@@ -4341,6 +4343,9 @@ bpstat_what (bpstat bs_head)
       else
 	bptype = bs->breakpoint_at->type;

+      if (bptype == bp_shlib_event)
+	shlib_event = 1;
+
       switch (bptype)
 	{
 	case bp_none:
@@ -4349,6 +4354,7 @@ bpstat_what (bpstat bs_head)
 	case bp_hardware_breakpoint:
 	case bp_until:
 	case bp_finish:
+	case bp_shlib_event:
 	  if (bs->stop)
 	    {
 	      if (bs->print)
@@ -4426,18 +4432,6 @@ bpstat_what (bpstat bs_head)
 		 This requires no further action.  */
 	    }
 	  break;
-	case bp_shlib_event:
-	  shlib_event = 1;
-
-	  /* If requested, stop when the dynamic linker notifies GDB
-	     of events.  This allows the user to get control and place
-	     breakpoints in initializer routines for dynamically
-	     loaded objects (among other things).  */
-	  if (stop_on_solib_events)
-	    this_action = BPSTAT_WHAT_STOP_NOISY;
-	  else
-	    this_action = BPSTAT_WHAT_SINGLE;
-	  break;
 	case bp_jit_event:
 	  jit_event = 1;
 	  this_action = BPSTAT_WHAT_SINGLE;
@@ -11144,8 +11138,17 @@ internal_bkpt_re_set (struct breakpoint *b)
 static void
 internal_bkpt_check_status (bpstat bs)
 {
-  /* We do not stop for these.  */
-  bs->stop = 0;
+  if (bs->breakpoint_at->type == bp_shlib_event)
+    {
+      /* If requested, stop when the dynamic linker notifies GDB of
+	 events.  This allows the user to get control and place
+	 breakpoints in initializer routines for dynamically loaded
+	 objects (among other things).  */
+      bs->stop = stop_on_solib_events;
+      bs->print = stop_on_solib_events;
+    }
+  else
+    bs->stop = 0;
 }

 static enum print_stop_action