[RFA] Stop infrun from tracking breakpoint insertion status.

[RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

The infrun.c module goes to great pains in order to maintain "breakpoints are
inserted" flag. However, it turns out that in a number of cases, it does
not really need to check that flag and remove_breakpoints does no harm of
no breakpoint is inserted; in a number of cases, the code actually is interested
in a specific breakpoint, and in remaining 3 cases where "breakpoints are inserted"
flag is essential for code logic, it's best to keep that flag in breakpoint.c.
No regressions. OK?

- Volodya

	The checks of breakpoints_inserted before calling remove_breakpoints
	are removed, as remove_breakpoint won't touch uninserted breakpoints.
	In a number of places, we're interested if a breakpoint is inserted
	at particular PC, and we now use breakpoint_inserted_here_p.  In a few
	places, breakpoints_inserted indicates if infrun.c expects breakpoints
	to be inserted in the target, and that state is now kept in breakpoint.c

	* breakpoint.h (breakpoints_meant_to_be_inserted): New
	declaration.
	* breakpoint.c (breakpoints_meant_to_be_inserted_p): New.
	(breakpoints_meant_to_be_inserted): New.
	* infrun.c (breakpoints_inserted): Remove.
	(resume): Don't check for breakpoints_inserted before
	remove_hw_watchpoints. Use breakpoint_inserted_here_p.
	(proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
	(handle_inferior_event): Don't use breakpoints_inserted.
	Use breakpoints_meant_to_be_inserted and
	breakpoints_inserted_here_p.
	(insert_step_resume_breakpoint_at_sal, keep_going): Use
	breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
	(normal_stop): Don't check for breakpoints_inserted.  Don't
	set breakpoints_inserted.
---
 gdb/breakpoint.c |    8 ++++++++
 gdb/breakpoint.h |    7 +++++++
 gdb/infrun.c     |   53 ++++++++++++++---------------------------------------
 3 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e81ec20..ef55a3d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -296,6 +296,8 @@ int breakpoint_count;
 /* Pointer to current exception event record */
 static struct exception_event_record *current_exception_event;
 
+static int breakpoints_meant_to_be_inserted_p;
+
 /* This function returns a pointer to the string representation of the
    pathname of the dynamically-linked library that has just been
    loaded.
@@ -1312,6 +1314,12 @@ remove_breakpoints (void)
 }
 
 int
+breakpoints_meant_to_be_inserted (void)
+{
+  return breakpoints_meant_to_be_inserted_p;
+}
+
+int
 remove_hw_watchpoints (void)
 {
   struct bp_location *b;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f13d41b..71515bc 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -862,4 +862,11 @@ extern int deprecated_remove_raw_breakpoint (void *);
    target.  */
 int watchpoints_triggered (struct target_waitstatus *);
 
+/* Returns non-zero if insert_breakpoints was previously called,
+   and remove_breakpoints was not called after that.
+   This function allows to figure out if we meant that all breakpoints
+   be inserted in inferior.  If so, any new breakpoints possibly
+   created must be inserted as well.  */
+extern int breakpoints_meant_to_be_inserted (void);
+
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 85d889a..5191e0f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -214,10 +214,6 @@ static unsigned char *signal_program;
 
 static struct cmd_list_element *stop_command;
 
-/* Nonzero if breakpoints are now inserted in the inferior.  */
-
-static int breakpoints_inserted;
-
 /* Function inferior was in as of last step command.  */
 
 static struct symbol *step_start_function;
@@ -519,7 +515,7 @@ resume (int step, enum target_signal sig)
      Work around the problem by removing hardware watchpoints if a step is
      requested, GDB will check for a hardware watchpoint trigger after the
      step anyway.  */
-  if (CANNOT_STEP_HW_WATCHPOINTS && step && breakpoints_inserted)
+  if (CANNOT_STEP_HW_WATCHPOINTS && step)
     remove_hw_watchpoints ();
 
 
@@ -634,7 +630,7 @@ a command like `return' or `jump' to continue execution."));
 	  /* Most targets can step a breakpoint instruction, thus
 	     executing it normally.  But if this one cannot, just
 	     continue and we will hit it anyway.  */
-	  if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+	  if (step && breakpoint_inserted_here_p (read_pc ()))
 	    step = 0;
 	}
       target_resume (resume_ptid, step, sig);
@@ -783,12 +779,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
        Continue it automatically and insert breakpoints then.  */
     trap_expected = 1;
   else
-    {
-      insert_breakpoints ();
-      /* If we get here there was no call to error() in 
-         insert breakpoints -- so they were inserted.  */
-      breakpoints_inserted = 1;
-    }
+    insert_breakpoints ();
 
   if (siggnal != TARGET_SIGNAL_DEFAULT)
     stop_signal = siggnal;
@@ -884,7 +875,6 @@ init_wait_for_inferior (void)
   /* These are meaningless until the first time through wait_for_inferior.  */
   prev_pc = 0;
 
-  breakpoints_inserted = 0;
   breakpoint_init_inferior (inf_starting);
 
   /* Don't confuse first call to proceed(). */
@@ -1334,10 +1324,8 @@ handle_inferior_event (struct execution_control_state *ecs)
 
 	  /* Remove breakpoints, SOLIB_ADD might adjust
 	     breakpoint addresses via breakpoint_re_set.  */
-	  breakpoints_were_inserted = breakpoints_inserted;
-	  if (breakpoints_inserted)
-	    remove_breakpoints ();
-	  breakpoints_inserted = 0;
+	  breakpoints_were_inserted = breakpoints_meant_to_be_inserted ();
+	  remove_breakpoints ();
 
 	  /* Check for any newly added shared libraries if we're
 	     supposed to be adding them automatically.  Switch
@@ -1381,10 +1369,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 
 	  /* Reinsert breakpoints and continue.  */
 	  if (breakpoints_were_inserted)
-	    {
-	      insert_breakpoints ();
-	      breakpoints_inserted = 1;
-	    }
+	    insert_breakpoints ();
 	}
 
       /* If we are skipping through a shell, or through shared library
@@ -1670,7 +1655,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
+      if (breakpoint_inserted_here_p (stop_pc))
 	{
 	  ecs->random_signal = 0;
 	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
@@ -1783,7 +1768,6 @@ handle_inferior_event (struct execution_control_state *ecs)
 	    }
 	  else
 	    {			/* Single step */
-	      breakpoints_inserted = 0;
 	      if (!ptid_equal (inferior_ptid, ecs->ptid))
 		context_switch (ecs);
 	      ecs->waiton_ptid = ecs->ptid;
@@ -1945,7 +1929,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      stack.  */
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      || (breakpoints_inserted
+      || (breakpoint_inserted_here_p (stop_pc)
 	  && (stop_signal == TARGET_SIGNAL_ILL
 	      || stop_signal == TARGET_SIGNAL_SEGV
 	      || stop_signal == TARGET_SIGNAL_EMT))
@@ -2076,8 +2060,8 @@ process_event_stop_test:
 	stop_signal = TARGET_SIGNAL_0;
 
       if (prev_pc == read_pc ()
-	  && !breakpoints_inserted
 	  && breakpoint_here_p (read_pc ())
+	  && !breakpoint_inserted_here_p (read_pc ())
 	  && step_resume_breakpoint == NULL)
 	{
 	  /* We were just starting a new sequence, attempting to
@@ -2150,7 +2134,6 @@ process_event_stop_test:
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME\n");
 	disable_longjmp_breakpoint ();
 	remove_breakpoints ();
-	breakpoints_inserted = 0;
 	if (!gdbarch_get_longjmp_target_p (current_gdbarch)
 	    || !gdbarch_get_longjmp_target (current_gdbarch,
 					    get_current_frame (), &jmp_buf_pc))
@@ -2176,7 +2159,6 @@ process_event_stop_test:
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_CLEAR_LONGJMP_RESUME\n");
 	remove_breakpoints ();
-	breakpoints_inserted = 0;
 	disable_longjmp_breakpoint ();
 	ecs->handling_longjmp = 0;	/* FIXME */
 	if (what.main_action == BPSTAT_WHAT_CLEAR_LONGJMP_RESUME)
@@ -2186,9 +2168,7 @@ process_event_stop_test:
       case BPSTAT_WHAT_SINGLE:
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
-	if (breakpoints_inserted)
-	  remove_breakpoints ();
-	breakpoints_inserted = 0;
+	remove_breakpoints ();
 	ecs->another_trap = 1;
 	/* Still need to check other stuff, at least the case
 	   where we are stepping and step out of the right range.  */
@@ -2250,7 +2230,6 @@ process_event_stop_test:
 	       to doing that.  */
 	    ecs->step_after_step_resume_breakpoint = 0;
 	    remove_breakpoints ();
-	    breakpoints_inserted = 0;
 	    ecs->another_trap = 1;
 	    keep_going (ecs);
 	    return;
@@ -2265,9 +2244,7 @@ process_event_stop_test:
 	  /* Remove breakpoints, we eventually want to step over the
 	     shlib event breakpoint, and SOLIB_ADD might adjust
 	     breakpoint addresses via breakpoint_re_set.  */
-	  if (breakpoints_inserted)
-	    remove_breakpoints ();
-	  breakpoints_inserted = 0;
+	  remove_breakpoints ();
 
 	  /* Check for any newly added shared libraries if we're
 	     supposed to be adding them automatically.  Switch
@@ -2870,7 +2847,7 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id,
 						     bp_step_resume);
-  if (breakpoints_inserted)
+  if (breakpoints_meant_to_be_inserted ())
     insert_breakpoints ();
 }
 
@@ -2970,7 +2947,7 @@ keep_going (struct execution_control_state *ecs)
 
          We're going to run this baby now!  */
 
-      if (!breakpoints_inserted && !ecs->another_trap)
+      if (!breakpoints_meant_to_be_inserted () && !ecs->another_trap)
 	{
 	  /* Stop stepping when inserting breakpoints
 	     has failed.  */
@@ -2979,7 +2956,6 @@ keep_going (struct execution_control_state *ecs)
 	      stop_stepping (ecs);
 	      return;
 	    }
-	  breakpoints_inserted = 1;
 	}
 
       trap_expected = ecs->another_trap;
@@ -3172,7 +3148,7 @@ normal_stop (void)
        gdbarch_decr_pc_after_break needs to just go away.  */
     deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());
 
-  if (target_has_execution && breakpoints_inserted)
+  if (target_has_execution)
     {
       if (remove_breakpoints ())
 	{
@@ -3183,7 +3159,6 @@ It might be running in another process.\n\
 Further execution is probably impossible.\n"));
 	}
     }
-  breakpoints_inserted = 0;
 
   /* Delete the breakpoint we stopped at, if it wants to be deleted.
      Delete any breakpoint that is to be deleted at the next stop.  */
-- 
1.5.3.5

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Ulrich Weigand]

Vladimir Prus wrote:

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e81ec20..ef55a3d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -296,6 +296,8 @@ int breakpoint_count;
>  /* Pointer to current exception event record */
>  static struct exception_event_record *current_exception_event;
>  
> +static int breakpoints_meant_to_be_inserted_p;
> +
>  /* This function returns a pointer to the string representation of the
>     pathname of the dynamically-linked library that has just been
>     loaded.
> @@ -1312,6 +1314,12 @@ remove_breakpoints (void)
>  }
>  
>  int
> +breakpoints_meant_to_be_inserted (void)
> +{
> +  return breakpoints_meant_to_be_inserted_p;
> +}
> +
> +int
>  remove_hw_watchpoints (void)
>  {
>    struct bp_location *b;


Is the breakpoints_meant_to_be_inserted_p variable actually ever
set to any non-zero value in this patch?

Bye,
Ulrich

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

Ulrich Weigand wrote:

> Vladimir Prus wrote:
> 
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index e81ec20..ef55a3d 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -296,6 +296,8 @@ int breakpoint_count;
>>  /* Pointer to current exception event record */
>>  static struct exception_event_record *current_exception_event;
>>  
>> +static int breakpoints_meant_to_be_inserted_p;
>> +
>>  /* This function returns a pointer to the string representation of the
>>     pathname of the dynamically-linked library that has just been
>>     loaded.
>> @@ -1312,6 +1314,12 @@ remove_breakpoints (void)
>>  }
>>  
>>  int
>> +breakpoints_meant_to_be_inserted (void)
>> +{
>> +  return breakpoints_meant_to_be_inserted_p;
>> +}
>> +
>> +int
>>  remove_hw_watchpoints (void)
>>  {
>>    struct bp_location *b;
> 
> 
> Is the breakpoints_meant_to_be_inserted_p variable actually ever
> set to any non-zero value in this patch?

Oops! It's not, which suggest that the three uses of that function are:
(i) not excercised by gdb testsuite and (ii) they are probably less
important than I originally though. 

I'll put the right assignemnts to breakpoints_meant_to_be_inserted_p
and look at the code in question again. 

Sorry,
Volodya

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

On Monday 19 November 2007 14:39:27 you wrote:
> Vladimir Prus wrote:
> 
> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > index e81ec20..ef55a3d 100644
> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -296,6 +296,8 @@ int breakpoint_count;
> >  /* Pointer to current exception event record */
> >  static struct exception_event_record *current_exception_event;
> >  
> > +static int breakpoints_meant_to_be_inserted_p;
> > +
> >  /* This function returns a pointer to the string representation of the
> >     pathname of the dynamically-linked library that has just been
> >     loaded.
> > @@ -1312,6 +1314,12 @@ remove_breakpoints (void)
> >  }
> >  
> >  int
> > +breakpoints_meant_to_be_inserted (void)
> > +{
> > +  return breakpoints_meant_to_be_inserted_p;
> > +}
> > +
> > +int
> >  remove_hw_watchpoints (void)
> >  {
> >    struct bp_location *b;
> 
> 
> Is the breakpoints_meant_to_be_inserted_p variable actually ever
> set to any non-zero value in this patch?

I have looked into why I got no failures with the previous version
of the patch. 

The use of breakpoints_meant_to_be_inserted in handle_inferiour_event, 
for the TARGET_WAITKIND_LOADED,  did not matter because 
TARGET_WAITKIND_LOADED is used by just a few targets.
And even if remote.c can use it, it does not do when 
using gdbserver, which makes it hard to test.

The reason that use in keep_going does not break anything is that
the intention of the code is to insert breakpoints, unless either
they are already inserted, or ecs->another_trap is non-zero. However,
insert_bp_location will immediately return if breakpoint location
is already inserted. Therefore, the "already inserted" check is
not necessary at all, and I removed it.

As for the use in insert_step_resume_breakpoint_at_sal, I must admit
I've got lost in the code again -- I don't know how previous version
of the patch did not cause any problems.

This version of patch makes breakpoints_meant_to_be_inserted
actually return 0 and 1 as needed, and adjust keep_going. OK?

- Volodya


	The checks of breakpoints_inserted before calling remove_breakpoints
	are removed, as remove_breakpoint won't touch uninserted breakpoints.
	In a number of places, we're interested if a breakpoint is inserted
	at particular PC, and we now use breakpoint_inserted_here_p.  In a few
	places, breakpoints_inserted indicates if infrun.c expects breakpoints
	to be inserted in the target, and that state is now kept in breakpoint.c

        * breakpoint.h (breakpoints_meant_to_be_inserted): New
        declaration.
        * breakpoint.c (breakpoints_meant_to_be_inserted_p): New.
        (breakpoints_meant_to_be_inserted): New.
        * infrun.c (breakpoints_inserted): Remove.
        (resume): Don't check for breakpoints_inserted before
        remove_hw_watchpoints. Use breakpoint_inserted_here_p.
        (proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
        (handle_inferior_event): Don't use breakpoints_inserted.
        Use breakpoints_meant_to_be_inserted and
        breakpoints_inserted_here_p.
        (insert_step_resume_breakpoint_at_sal, keep_going): Use
        breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
        (normal_stop): Don't check for breakpoints_inserted.  Don't
        set breakpoints_inserted.
        (keep_going): Don't check for breakpoints_inserted.
---
 gdb/breakpoint.c |   12 +++++++++++
 gdb/breakpoint.h |    7 ++++++
 gdb/infrun.c     |   59 +++++++++++++++++------------------------------------
 3 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 81614aa..d2bd232 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -296,6 +296,8 @@ int breakpoint_count;
 /* Pointer to current exception event record */
 static struct exception_event_record *current_exception_event;
 
+static int breakpoints_meant_to_be_inserted_p;
+
 /* This function returns a pointer to the string representation of the
    pathname of the dynamically-linked library that has just been
    loaded.
@@ -1235,6 +1237,8 @@ insert_breakpoints (void)
   struct ui_file *tmp_error_stream = mem_fileopen ();
   make_cleanup_ui_file_delete (tmp_error_stream);
 
+  breakpoints_meant_to_be_inserted_p = 1;
+
   /* Explicitly mark the warning -- this will only be printed if
      there was an error.  */
   fprintf_unfiltered (tmp_error_stream, "Warning:\n");
@@ -1299,6 +1303,8 @@ remove_breakpoints (void)
   struct bp_location *b;
   int val;
 
+  breakpoints_meant_to_be_inserted_p = 0;
+
   ALL_BP_LOCATIONS (b)
   {
     if (b->inserted)
@@ -1312,6 +1318,12 @@ remove_breakpoints (void)
 }
 
 int
+breakpoints_meant_to_be_inserted (void)
+{
+  return breakpoints_meant_to_be_inserted_p;
+}
+
+int
 remove_hw_watchpoints (void)
 {
   struct bp_location *b;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e4aa72a..236cd7b 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -854,4 +854,11 @@ extern int deprecated_remove_raw_breakpoint (void *);
    target.  */
 int watchpoints_triggered (struct target_waitstatus *);
 
+/* Returns non-zero if insert_breakpoints was previously called,
+   and remove_breakpoints was not called after that.
+   This function allows to figure out if we meant that all breakpoints
+   be inserted in inferior.  If so, any new breakpoints possibly
+   created must be inserted as well.  */
+extern int breakpoints_meant_to_be_inserted (void);
+
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 85d889a..9a1c24f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -214,10 +214,6 @@ static unsigned char *signal_program;
 
 static struct cmd_list_element *stop_command;
 
-/* Nonzero if breakpoints are now inserted in the inferior.  */
-
-static int breakpoints_inserted;
-
 /* Function inferior was in as of last step command.  */
 
 static struct symbol *step_start_function;
@@ -519,7 +515,7 @@ resume (int step, enum target_signal sig)
      Work around the problem by removing hardware watchpoints if a step is
      requested, GDB will check for a hardware watchpoint trigger after the
      step anyway.  */
-  if (CANNOT_STEP_HW_WATCHPOINTS && step && breakpoints_inserted)
+  if (CANNOT_STEP_HW_WATCHPOINTS && step)
     remove_hw_watchpoints ();
 
 
@@ -634,7 +630,7 @@ a command like `return' or `jump' to continue execution."));
          /* Most targets can step a breakpoint instruction, thus
             executing it normally.  But if this one cannot, just
             continue and we will hit it anyway.  */
-         if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+         if (step && breakpoint_inserted_here_p (read_pc ()))
            step = 0;
        }
       target_resume (resume_ptid, step, sig);
@@ -783,12 +779,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
        Continue it automatically and insert breakpoints then.  */
     trap_expected = 1;
   else
-    {
-      insert_breakpoints ();
-      /* If we get here there was no call to error() in 
-         insert breakpoints -- so they were inserted.  */
-      breakpoints_inserted = 1;
-    }
+    insert_breakpoints ();
 
   if (siggnal != TARGET_SIGNAL_DEFAULT)
     stop_signal = siggnal;
@@ -884,7 +875,6 @@ init_wait_for_inferior (void)
   /* These are meaningless until the first time through wait_for_inferior.  */
   prev_pc = 0;
 
-  breakpoints_inserted = 0;
   breakpoint_init_inferior (inf_starting);
 
   /* Don't confuse first call to proceed(). */
@@ -1334,10 +1324,8 @@ handle_inferior_event (struct execution_control_state *ecs)
 
          /* Remove breakpoints, SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         breakpoints_were_inserted = breakpoints_inserted;
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         breakpoints_were_inserted = breakpoints_meant_to_be_inserted ();
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -1381,10 +1369,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 
          /* Reinsert breakpoints and continue.  */
          if (breakpoints_were_inserted)
-           {
-             insert_breakpoints ();
-             breakpoints_inserted = 1;
-           }
+           insert_breakpoints ();
        }
 
       /* If we are skipping through a shell, or through shared library
@@ -1670,7 +1655,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
+      if (breakpoint_inserted_here_p (stop_pc))
        {
          ecs->random_signal = 0;
          if (!breakpoint_thread_match (stop_pc, ecs->ptid))
@@ -1783,7 +1768,6 @@ handle_inferior_event (struct execution_control_state *ecs)
            }
          else
            {                   /* Single step */
-             breakpoints_inserted = 0;
              if (!ptid_equal (inferior_ptid, ecs->ptid))
                context_switch (ecs);
              ecs->waiton_ptid = ecs->ptid;
@@ -1945,7 +1929,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      stack.  */
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      || (breakpoints_inserted
+      || (breakpoint_inserted_here_p (stop_pc)
          && (stop_signal == TARGET_SIGNAL_ILL
              || stop_signal == TARGET_SIGNAL_SEGV
              || stop_signal == TARGET_SIGNAL_EMT))
@@ -2076,8 +2060,8 @@ process_event_stop_test:
        stop_signal = TARGET_SIGNAL_0;
 
       if (prev_pc == read_pc ()
-         && !breakpoints_inserted
          && breakpoint_here_p (read_pc ())
+         && !breakpoint_inserted_here_p (read_pc ())
          && step_resume_breakpoint == NULL)
        {
          /* We were just starting a new sequence, attempting to
@@ -2150,7 +2134,6 @@ process_event_stop_test:
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME\n");
        disable_longjmp_breakpoint ();
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        if (!gdbarch_get_longjmp_target_p (current_gdbarch)
            || !gdbarch_get_longjmp_target (current_gdbarch,
                                            get_current_frame (), &jmp_buf_pc))
@@ -2176,7 +2159,6 @@ process_event_stop_test:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_CLEAR_LONGJMP_RESUME\n");
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        disable_longjmp_breakpoint ();
        ecs->handling_longjmp = 0;      /* FIXME */
        if (what.main_action == BPSTAT_WHAT_CLEAR_LONGJMP_RESUME)
@@ -2186,9 +2168,7 @@ process_event_stop_test:
       case BPSTAT_WHAT_SINGLE:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
-       if (breakpoints_inserted)
-         remove_breakpoints ();
-       breakpoints_inserted = 0;
+       remove_breakpoints ();
        ecs->another_trap = 1;
        /* Still need to check other stuff, at least the case
           where we are stepping and step out of the right range.  */
@@ -2250,7 +2230,6 @@ process_event_stop_test:
               to doing that.  */
            ecs->step_after_step_resume_breakpoint = 0;
            remove_breakpoints ();
-           breakpoints_inserted = 0;
            ecs->another_trap = 1;
            keep_going (ecs);
            return;
@@ -2265,9 +2244,7 @@ process_event_stop_test:
          /* Remove breakpoints, we eventually want to step over the
             shlib event breakpoint, and SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -2870,7 +2847,7 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id,
                                                     bp_step_resume);
-  if (breakpoints_inserted)
+  if (breakpoints_meant_to_be_inserted ())
     insert_breakpoints ();
 }
 
@@ -2968,9 +2945,13 @@ keep_going (struct execution_control_state *ecs)
          The signal was SIGTRAP, e.g. it was our signal, but we
          decided we should resume from it.
 
-         We're going to run this baby now!  */
+         We're going to run this baby now!  
 
-      if (!breakpoints_inserted && !ecs->another_trap)
+        Note that insert_breakpoints won't try to re-insert
+        already inserted breakpoints.  Therefore, we don't
+        care if breakpoints were already inserted, or not.  */
+      
+      if (!ecs->another_trap)
        {
          /* Stop stepping when inserting breakpoints
             has failed.  */
@@ -2979,7 +2960,6 @@ keep_going (struct execution_control_state *ecs)
              stop_stepping (ecs);
              return;
            }
-         breakpoints_inserted = 1;
        }
 
       trap_expected = ecs->another_trap;
@@ -3172,7 +3152,7 @@ normal_stop (void)
        gdbarch_decr_pc_after_break needs to just go away.  */
     deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());
 
-  if (target_has_execution && breakpoints_inserted)
+  if (target_has_execution)
     {
       if (remove_breakpoints ())
        {
@@ -3183,7 +3163,6 @@ It might be running in another process.\n\
 Further execution is probably impossible.\n"));
        }
     }
-  breakpoints_inserted = 0;
 
   /* Delete the breakpoint we stopped at, if it wants to be deleted.
      Delete any breakpoint that is to be deleted at the next stop.  */

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Ulrich Weigand]

Vladimir Prus wrote:

> The use of breakpoints_meant_to_be_inserted in handle_inferiour_event, 
> for the TARGET_WAITKIND_LOADED,  did not matter because 
> TARGET_WAITKIND_LOADED is used by just a few targets.
> And even if remote.c can use it, it does not do when 
> using gdbserver, which makes it hard to test.

Hmmm, if it helps, I could run a test on AIX, which does use
TARGET_WAITKIND_LOADED.

In any case, at this point breakpoints *must* be inserted -- the
very next thing the code does is to call
          resume (0, TARGET_SIGNAL_0);
so if breakpoints were not inserted, we'd just run the inferior
to completion now.

So I think the check is not necessary, and we should simply
unconditionally insert breakpoints at this point. 
 
> The reason that use in keep_going does not break anything is that
> the intention of the code is to insert breakpoints, unless either
> they are already inserted, or ecs->another_trap is non-zero. However,
> insert_bp_location will immediately return if breakpoint location
> is already inserted. Therefore, the "already inserted" check is
> not necessary at all, and I removed it.

OK, agreed.
 
> As for the use in insert_step_resume_breakpoint_at_sal, I must admit
> I've got lost in the code again -- I don't know how previous version
> of the patch did not cause any problems.

I think this is because calls to insert_step_resume_breakpoint_at_sal
are always followed by calls to keep_going -- and due to the behaviour
you described above, with your old patch keep_going would always 
insert the step-resume breakpoint anyway.

In fact, I think with the change to keep_going to always insert all
breakpoints there is no need for insert_step_resume_breakpoint_at_sal
to call insert_breakpoints at all anymore.  This should probably
best be removed.


If I've counted correctly, with the changes I've described we've
completely eliminated the need for breakpoints_meant_to_be_inserted.
Would you mind updating your patch accordingly?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

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

On Thursday 22 November 2007 03:49:22 you wrote:
> Vladimir Prus wrote:
> 
> > The use of breakpoints_meant_to_be_inserted in handle_inferiour_event, 
> > for the TARGET_WAITKIND_LOADED,  did not matter because 
> > TARGET_WAITKIND_LOADED is used by just a few targets.
> > And even if remote.c can use it, it does not do when 
> > using gdbserver, which makes it hard to test.
> 
> Hmmm, if it helps, I could run a test on AIX, which does use
> TARGET_WAITKIND_LOADED.

That would surely help in convincing ourself the patch don't
break anything.

> In any case, at this point breakpoints *must* be inserted -- the
> very next thing the code does is to call
>           resume (0, TARGET_SIGNAL_0);
> so if breakpoints were not inserted, we'd just run the inferior
> to completion now.

Yes, I think you're right.

> So I think the check is not necessary, and we should simply
> unconditionally insert breakpoints at this point. 
>  
> > The reason that use in keep_going does not break anything is that
> > the intention of the code is to insert breakpoints, unless either
> > they are already inserted, or ecs->another_trap is non-zero. However,
> > insert_bp_location will immediately return if breakpoint location
> > is already inserted. Therefore, the "already inserted" check is
> > not necessary at all, and I removed it.
> 
> OK, agreed.
>  
> > As for the use in insert_step_resume_breakpoint_at_sal, I must admit
> > I've got lost in the code again -- I don't know how previous version
> > of the patch did not cause any problems.
> 
> I think this is because calls to insert_step_resume_breakpoint_at_sal
> are always followed by calls to keep_going -- and due to the behaviour
> you described above, with your old patch keep_going would always 
> insert the step-resume breakpoint anyway.
> 
> In fact, I think with the change to keep_going to always insert all
> breakpoints there is no need for insert_step_resume_breakpoint_at_sal
> to call insert_breakpoints at all anymore.  This should probably
> best be removed.

I've checked, and indeed, keep_going is called in all cases 
insert_step_resume_breakpoint_at_sal is called. 

> If I've counted correctly, with the changes I've described we've
> completely eliminated the need for breakpoints_meant_to_be_inserted.

Yeah, that's even better result that I expected.

> Would you mind updating your patch accordingly?

Sure, the revised patch is below. The delta against previous
patch version is attached. OK?

Thanks,
Volodya

	The checks of breakpoints_inserted before calling remove_breakpoints
	are removed, as remove_breakpoint won't touch uninserted breakpoints.
	In a number of places, we're interested if a breakpoint is inserted
	at particular PC, and we now use breakpoint_inserted_here_p.  In a few
	places, insert_breakpoints can be called unconditionally, since it
	won't try to insert already inserted breakpoint.

        * infrun.c (breakpoints_inserted): Remove.
        (resume): Don't check for breakpoints_inserted before
        remove_hw_watchpoints. Use breakpoint_inserted_here_p.
        (proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
        (handle_inferior_event): Don't use breakpoints_inserted.
        Use breakpoints_meant_to_be_inserted and
        breakpoints_inserted_here_p.
        (insert_step_resume_breakpoint_at_sal, keep_going): Use
        breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
        (normal_stop): Don't check for breakpoints_inserted.  Don't
        set breakpoints_inserted.
        (keep_going): Don't check for breakpoints_inserted.
        (insert_step_resume_breakpoint_at_sal): Don't insert
        breakpoints
---
 gdb/infrun.c |   59 ++++++++++++++++-----------------------------------------
 1 files changed, 17 insertions(+), 42 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 85d889a..08b0cf3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -214,10 +214,6 @@ static unsigned char *signal_program;
 
 static struct cmd_list_element *stop_command;
 
-/* Nonzero if breakpoints are now inserted in the inferior.  */
-
-static int breakpoints_inserted;
-
 /* Function inferior was in as of last step command.  */
 
 static struct symbol *step_start_function;
@@ -519,7 +515,7 @@ resume (int step, enum target_signal sig)
      Work around the problem by removing hardware watchpoints if a step is
      requested, GDB will check for a hardware watchpoint trigger after the
      step anyway.  */
-  if (CANNOT_STEP_HW_WATCHPOINTS && step && breakpoints_inserted)
+  if (CANNOT_STEP_HW_WATCHPOINTS && step)
     remove_hw_watchpoints ();
 
 
@@ -634,7 +630,7 @@ a command like `return' or `jump' to continue execution."));
          /* Most targets can step a breakpoint instruction, thus
             executing it normally.  But if this one cannot, just
             continue and we will hit it anyway.  */
-         if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+         if (step && breakpoint_inserted_here_p (read_pc ()))
            step = 0;
        }
       target_resume (resume_ptid, step, sig);
@@ -783,12 +779,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
        Continue it automatically and insert breakpoints then.  */
     trap_expected = 1;
   else
-    {
-      insert_breakpoints ();
-      /* If we get here there was no call to error() in 
-         insert breakpoints -- so they were inserted.  */
-      breakpoints_inserted = 1;
-    }
+    insert_breakpoints ();
 
   if (siggnal != TARGET_SIGNAL_DEFAULT)
     stop_signal = siggnal;
@@ -884,7 +875,6 @@ init_wait_for_inferior (void)
   /* These are meaningless until the first time through wait_for_inferior.  */
   prev_pc = 0;
 
-  breakpoints_inserted = 0;
   breakpoint_init_inferior (inf_starting);
 
   /* Don't confuse first call to proceed(). */
@@ -1334,10 +1324,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 
          /* Remove breakpoints, SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         breakpoints_were_inserted = breakpoints_inserted;
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -1380,11 +1367,7 @@ handle_inferior_event (struct execution_control_state *ecs)
             for "catch load".  */
 
          /* Reinsert breakpoints and continue.  */
-         if (breakpoints_were_inserted)
-           {
-             insert_breakpoints ();
-             breakpoints_inserted = 1;
-           }
+         insert_breakpoints ();
        }
 
       /* If we are skipping through a shell, or through shared library
@@ -1670,7 +1653,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
+      if (breakpoint_inserted_here_p (stop_pc))
        {
          ecs->random_signal = 0;
          if (!breakpoint_thread_match (stop_pc, ecs->ptid))
@@ -1783,7 +1766,6 @@ handle_inferior_event (struct execution_control_state *ecs)
            }
          else
            {                   /* Single step */
-             breakpoints_inserted = 0;
              if (!ptid_equal (inferior_ptid, ecs->ptid))
                context_switch (ecs);
              ecs->waiton_ptid = ecs->ptid;
@@ -1945,7 +1927,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      stack.  */
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      || (breakpoints_inserted
+      || (breakpoint_inserted_here_p (stop_pc)
          && (stop_signal == TARGET_SIGNAL_ILL
              || stop_signal == TARGET_SIGNAL_SEGV
              || stop_signal == TARGET_SIGNAL_EMT))
@@ -2076,8 +2058,8 @@ process_event_stop_test:
        stop_signal = TARGET_SIGNAL_0;
 
       if (prev_pc == read_pc ()
-         && !breakpoints_inserted
          && breakpoint_here_p (read_pc ())
+         && !breakpoint_inserted_here_p (read_pc ())
          && step_resume_breakpoint == NULL)
        {
          /* We were just starting a new sequence, attempting to
@@ -2150,7 +2132,6 @@ process_event_stop_test:
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME\n");
        disable_longjmp_breakpoint ();
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        if (!gdbarch_get_longjmp_target_p (current_gdbarch)
            || !gdbarch_get_longjmp_target (current_gdbarch,
                                            get_current_frame (), &jmp_buf_pc))
@@ -2176,7 +2157,6 @@ process_event_stop_test:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_CLEAR_LONGJMP_RESUME\n");
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        disable_longjmp_breakpoint ();
        ecs->handling_longjmp = 0;      /* FIXME */
        if (what.main_action == BPSTAT_WHAT_CLEAR_LONGJMP_RESUME)
@@ -2186,9 +2166,7 @@ process_event_stop_test:
       case BPSTAT_WHAT_SINGLE:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
-       if (breakpoints_inserted)
-         remove_breakpoints ();
-       breakpoints_inserted = 0;
+       remove_breakpoints ();
        ecs->another_trap = 1;
        /* Still need to check other stuff, at least the case
           where we are stepping and step out of the right range.  */
@@ -2250,7 +2228,6 @@ process_event_stop_test:
               to doing that.  */
            ecs->step_after_step_resume_breakpoint = 0;
            remove_breakpoints ();
-           breakpoints_inserted = 0;
            ecs->another_trap = 1;
            keep_going (ecs);
            return;
@@ -2265,9 +2242,7 @@ process_event_stop_test:
          /* Remove breakpoints, we eventually want to step over the
             shlib event breakpoint, and SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -2870,8 +2845,6 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id,
                                                     bp_step_resume);
-  if (breakpoints_inserted)
-    insert_breakpoints ();
 }
 
 /* Insert a "step-resume breakpoint" at RETURN_FRAME.pc.  This is used
@@ -2968,9 +2941,13 @@ keep_going (struct execution_control_state *ecs)
          The signal was SIGTRAP, e.g. it was our signal, but we
          decided we should resume from it.
 
-         We're going to run this baby now!  */
+         We're going to run this baby now!  
 
-      if (!breakpoints_inserted && !ecs->another_trap)
+        Note that insert_breakpoints won't try to re-insert
+        already inserted breakpoints.  Therefore, we don't
+        care if breakpoints were already inserted, or not.  */
+      
+      if (!ecs->another_trap)
        {
          /* Stop stepping when inserting breakpoints
             has failed.  */
@@ -2979,7 +2956,6 @@ keep_going (struct execution_control_state *ecs)
              stop_stepping (ecs);
              return;
            }
-         breakpoints_inserted = 1;
        }
 
       trap_expected = ecs->another_trap;
@@ -3172,7 +3148,7 @@ normal_stop (void)
        gdbarch_decr_pc_after_break needs to just go away.  */
     deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());
 
-  if (target_has_execution && breakpoints_inserted)
+  if (target_has_execution)
     {
       if (remove_breakpoints ())
        {
@@ -3183,7 +3159,6 @@ It might be running in another process.\n\
 Further execution is probably impossible.\n"));
        }
     }
-  breakpoints_inserted = 0;
 
   /* Delete the breakpoint we stopped at, if it wants to be deleted.
      Delete any breakpoint that is to be deleted at the next stop.  */
-- 
1.5.3.5

[-- Attachment #2: delta.diff --]
[-- Type: text/x-diff, Size: 3028 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d2bd232..81614aa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -296,8 +296,6 @@ int breakpoint_count;
 /* Pointer to current exception event record */
 static struct exception_event_record *current_exception_event;
 
-static int breakpoints_meant_to_be_inserted_p;
-
 /* This function returns a pointer to the string representation of the
    pathname of the dynamically-linked library that has just been
    loaded.
@@ -1237,8 +1235,6 @@ insert_breakpoints (void)
   struct ui_file *tmp_error_stream = mem_fileopen ();
   make_cleanup_ui_file_delete (tmp_error_stream);
 
-  breakpoints_meant_to_be_inserted_p = 1;
-
   /* Explicitly mark the warning -- this will only be printed if
      there was an error.  */
   fprintf_unfiltered (tmp_error_stream, "Warning:\n");
@@ -1303,8 +1299,6 @@ remove_breakpoints (void)
   struct bp_location *b;
   int val;
 
-  breakpoints_meant_to_be_inserted_p = 0;
-
   ALL_BP_LOCATIONS (b)
   {
     if (b->inserted)
@@ -1318,12 +1312,6 @@ remove_breakpoints (void)
 }
 
 int
-breakpoints_meant_to_be_inserted (void)
-{
-  return breakpoints_meant_to_be_inserted_p;
-}
-
-int
 remove_hw_watchpoints (void)
 {
   struct bp_location *b;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 236cd7b..e4aa72a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -854,11 +854,4 @@ extern int deprecated_remove_raw_breakpoint (void *);
    target.  */
 int watchpoints_triggered (struct target_waitstatus *);
 
-/* Returns non-zero if insert_breakpoints was previously called,
-   and remove_breakpoints was not called after that.
-   This function allows to figure out if we meant that all breakpoints
-   be inserted in inferior.  If so, any new breakpoints possibly
-   created must be inserted as well.  */
-extern int breakpoints_meant_to_be_inserted (void);
-
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9a1c24f..08b0cf3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1324,7 +1324,6 @@ handle_inferior_event (struct execution_control_state *ecs)
 
 	  /* Remove breakpoints, SOLIB_ADD might adjust
 	     breakpoint addresses via breakpoint_re_set.  */
-	  breakpoints_were_inserted = breakpoints_meant_to_be_inserted ();
 	  remove_breakpoints ();
 
 	  /* Check for any newly added shared libraries if we're
@@ -1368,8 +1367,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	     for "catch load".  */
 
 	  /* Reinsert breakpoints and continue.  */
-	  if (breakpoints_were_inserted)
-	    insert_breakpoints ();
+	  insert_breakpoints ();
 	}
 
       /* If we are skipping through a shell, or through shared library
@@ -2847,8 +2845,6 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id,
 						     bp_step_resume);
-  if (breakpoints_meant_to_be_inserted ())
-    insert_breakpoints ();
 }
 
 /* Insert a "step-resume breakpoint" at RETURN_FRAME.pc.  This is used

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Ulrich Weigand]

Vladimir Prus wrote:

> On Thursday 22 November 2007 03:49:22 you wrote:
> > Hmmm, if it helps, I could run a test on AIX, which does use
> > TARGET_WAITKIND_LOADED.
> 
> That would surely help in convincing ourself the patch don't
> break anything.

I did a test run on AIX now, and unfortunately it did break.

I didn't look into the failure in detail, but apparently it
is unrelated to TARGET_WAITKIND_LOADED, but rather to software
single-step support:

rios2 10$ ./gdb testsuite/gdb.base/all-types
GNU gdb 6.7.50.20071126-cvs
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "powerpc-ibm-aix5.3.0.0"...
Setting up the environment for debugging gdb.
Function "internal_error" not defined.
Make breakpoint pending on future shared library load? (y or [n]) [answered N; input not from terminal]
Function "info_command" not defined.
Make breakpoint pending on future shared library load? (y or [n]) [answered N; input not from terminal]
/c1dje/c1dje/gdb-head-build/gdb/.gdbinit:8: Error in sourced command file:
No breakpoint number 0.
(gdb) start
Breakpoint 1 at 0x1000037c: file /c1dje/c1dje/gdb-head/gdb/testsuite/gdb.base/all-types.c, line 35.
Starting program: /c1dje/c1dje/gdb-head-build/gdb/testsuite/gdb.base/all-types
main () at /c1dje/c1dje/gdb-head/gdb/testsuite/gdb.base/all-types.c:35
35          dummy();
(gdb) n

Program exited normally.
(gdb) start
Breakpoint 2 at 0x1000037c: file /c1dje/c1dje/gdb-head/gdb/testsuite/gdb.base/all-types.c, line 35.
Starting program: /c1dje/c1dje/gdb-head-build/gdb/testsuite/gdb.base/all-types
main () at /c1dje/c1dje/gdb-head/gdb/testsuite/gdb.base/all-types.c:35
35          dummy();
(gdb) n
/c1dje/c1dje/gdb-head/gdb/breakpoint.c:8140: internal-error: insert_single_step_breakpoint: Assertion `single_step_breakpoints[1] == NULL' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.


Note how not only single-step does not work, but it appears to leave
its data structures in an inconsisten state to that a subsequent attempt
to use single-step breakpoints triggers an internal error ...

Can you have a look?

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

On Monday 26 November 2007 18:24:56 Ulrich Weigand wrote:
> Vladimir Prus wrote:
> 
> > On Thursday 22 November 2007 03:49:22 you wrote:
> > > Hmmm, if it helps, I could run a test on AIX, which does use
> > > TARGET_WAITKIND_LOADED.
> > 
> > That would surely help in convincing ourself the patch don't
> > break anything.
> 
> I did a test run on AIX now, and unfortunately it did break.
> 
> I didn't look into the failure in detail, but apparently it
> is unrelated to TARGET_WAITKIND_LOADED, but rather to software
> single-step support:

There were in fact two distinct problems. The first was
in my previous patch to document infrun logic -- while the
patch was meant to have no logic change, the case of 
stepping over breakpoint using software single step was
messed up. This patch fixes the problem -- it was tested
on arm-linux/qemu, which uses software single step, with
no test result changes. OK?

- Volodya

        * infrun.c (resume): Set right thread
        even if stepping over breakpoint using software
        single step.
---
 gdb/infrun.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 85d889a..00cd2a5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -605,7 +605,8 @@ a command like `return' or `jump' to continue execution."));
          resume_ptid = inferior_ptid;
        }
 
-      if (step && breakpoint_here_p (read_pc ())
+      if ((step || singlestep_breakpoints_inserted_p)
+         && breakpoint_here_p (read_pc ())
          && !breakpoint_inserted_here_p (read_pc ()))
        {
          /* We're stepping, have breakpoint at PC, and it's 
-- 
1.5.3.5

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Ulrich Weigand]

Vladimir Prus wrote:

> There were in fact two distinct problems. The first was
> in my previous patch to document infrun logic -- while the
> patch was meant to have no logic change, the case of 
> stepping over breakpoint using software single step was
> messed up. This patch fixes the problem -- it was tested
> on arm-linux/qemu, which uses software single step, with
> no test result changes. OK?

Ah, right.  Sorry for missing that.

>         * infrun.c (resume): Set right thread
>         even if stepping over breakpoint using software
>         single step.

This is OK.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

On Tuesday 27 November 2007 21:14:12 Ulrich Weigand wrote:
> Vladimir Prus wrote:
> 
> > There were in fact two distinct problems. The first was
> > in my previous patch to document infrun logic -- while the
> > patch was meant to have no logic change, the case of 
> > stepping over breakpoint using software single step was
> > messed up. This patch fixes the problem -- it was tested
> > on arm-linux/qemu, which uses software single step, with
> > no test result changes. OK?
> 
> Ah, right.  Sorry for missing that.
> 
> >         * infrun.c (resume): Set right thread
> >         even if stepping over breakpoint using software
> >         single step.
> 
> This is OK.

Thanks. This one is checked in.

- Volodya

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

On Monday 26 November 2007 18:24:56 Ulrich Weigand wrote:
> Vladimir Prus wrote:
> 
> > On Thursday 22 November 2007 03:49:22 you wrote:
> > > Hmmm, if it helps, I could run a test on AIX, which does use
> > > TARGET_WAITKIND_LOADED.
> > 
> > That would surely help in convincing ourself the patch don't
> > break anything.
> 
> I did a test run on AIX now, and unfortunately it did break.
> 
> I didn't look into the failure in detail, but apparently it
> is unrelated to TARGET_WAITKIND_LOADED, but rather to software
> single-step support:

The second problem was that breakpoint_inserted_here_p checks for
both ordinary breakpoints and single step breakpoints (which don't
go on bp_location_chain). In most cases, it does not matter, but
in one place the code tries to figure if we're stopped on a single
step breakpoint or an ordinary one, and surely, breakpoint_inserted_here_p
returning 1 for both cases was not good.

The attached revision of the patch introduces a separate
regular_breakpoint_inserted_here_p function. No regressions on
either x86 or arm-linux. OK?

- Volodya

	The checks of breakpoints_inserted before calling remove_breakpoints
	are removed, as remove_breakpoint won't touch uninserted breakpoints.
	In a number of places, we're interested if a breakpoint is inserted
	at particular PC, and we now use breakpoint_inserted_here_p.  In a few
	places, insert_breakpoints can be called unconditionally, since it
	won't try to insert already inserted breakpoint.

        * breakpoint.h (regular_breakpoint_inserted_here_p): New
        declaration.
        * breakpoint.c (regular_breakpoint_inserted_here_p): New.
        (breakpoint_inserted_here_p): Use
        regular_breakpoint_inserted_here_p.
        * infrun.c (breakpoints_inserted): Remove.
        (resume): Don't check for breakpoints_inserted before
        remove_hw_watchpoints. Use breakpoint_inserted_here_p.
        (proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
        (handle_inferior_event): Don't use breakpoints_inserted.
        Use breakpoints_meant_to_be_inserted and
        breakpoints_inserted_here_p.
        (insert_step_resume_breakpoint_at_sal, keep_going): Use
        breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
        (normal_stop): Don't check for breakpoints_inserted.  Don't
        set breakpoints_inserted.
        (keep_going): Don't check for breakpoints_inserted.
        (insert_step_resume_breakpoint_at_sal): Don't insert
        breakpoints
---
 gdb/breakpoint.c |   21 ++++++++++++++----
 gdb/breakpoint.h |    2 +
 gdb/infrun.c     |   59 +++++++++++++++--------------------------------------
 3 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2203f6e..2717302 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1757,12 +1757,13 @@ breakpoint_here_p (CORE_ADDR pc)
 }
 
 
-/* breakpoint_inserted_here_p (PC) is just like breakpoint_here_p(),
-   but it only returns true if there is actually a breakpoint inserted
-   at PC.  */
+/* Returns non-zero if there's a breakpoint inserted at PC, which is
+   inserted using regular breakpoint_chain/bp_location_chain mechanism.
+   This does not check for single-step breakpoints, which are
+   inserted and removed using direct target manipulation.  */
 
 int
-breakpoint_inserted_here_p (CORE_ADDR pc)
+regular_breakpoint_inserted_here_p (CORE_ADDR pc)
 {
   const struct bp_location *bpt;
 
@@ -1783,8 +1784,18 @@ breakpoint_inserted_here_p (CORE_ADDR pc)
            return 1;
        }
     }
+  return 0;
+}
+
+/* Returns non-zero iff there's either regular breakpoint
+   or a single step breakpoint inserted at PC.  */
+
+int
+breakpoint_inserted_here_p (CORE_ADDR pc)
+{
+  if (regular_breakpoint_inserted_here_p (pc))
+    return 1;
 
-  /* Also check for software single-step breakpoints.  */
   if (single_step_breakpoint_inserted_here_p (pc))
     return 1;
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e4aa72a..4b84502 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -674,6 +674,8 @@ extern enum breakpoint_here breakpoint_here_p (CORE_ADDR);
 
 extern int breakpoint_inserted_here_p (CORE_ADDR);
 
+extern int regular_breakpoint_inserted_here_p (CORE_ADDR);
+
 extern int software_breakpoint_inserted_here_p (CORE_ADDR);
 
 extern int breakpoint_thread_match (CORE_ADDR, ptid_t);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 00cd2a5..c357f27 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -214,10 +214,6 @@ static unsigned char *signal_program;
 
 static struct cmd_list_element *stop_command;
 
-/* Nonzero if breakpoints are now inserted in the inferior.  */
-
-static int breakpoints_inserted;
-
 /* Function inferior was in as of last step command.  */
 
 static struct symbol *step_start_function;
@@ -519,7 +515,7 @@ resume (int step, enum target_signal sig)
      Work around the problem by removing hardware watchpoints if a step is
      requested, GDB will check for a hardware watchpoint trigger after the
      step anyway.  */
-  if (CANNOT_STEP_HW_WATCHPOINTS && step && breakpoints_inserted)
+  if (CANNOT_STEP_HW_WATCHPOINTS && step)
     remove_hw_watchpoints ();
 
 
@@ -635,7 +631,7 @@ a command like `return' or `jump' to continue execution."));
          /* Most targets can step a breakpoint instruction, thus
             executing it normally.  But if this one cannot, just
             continue and we will hit it anyway.  */
-         if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+         if (step && breakpoint_inserted_here_p (read_pc ()))
            step = 0;
        }
       target_resume (resume_ptid, step, sig);
@@ -784,12 +780,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
        Continue it automatically and insert breakpoints then.  */
     trap_expected = 1;
   else
-    {
-      insert_breakpoints ();
-      /* If we get here there was no call to error() in 
-         insert breakpoints -- so they were inserted.  */
-      breakpoints_inserted = 1;
-    }
+    insert_breakpoints ();
 
   if (siggnal != TARGET_SIGNAL_DEFAULT)
     stop_signal = siggnal;
@@ -885,7 +876,6 @@ init_wait_for_inferior (void)
   /* These are meaningless until the first time through wait_for_inferior.  */
   prev_pc = 0;
 
-  breakpoints_inserted = 0;
   breakpoint_init_inferior (inf_starting);
 
   /* Don't confuse first call to proceed(). */
@@ -1335,10 +1325,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 
          /* Remove breakpoints, SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         breakpoints_were_inserted = breakpoints_inserted;
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -1381,11 +1368,7 @@ handle_inferior_event (struct execution_control_state *ecs)
             for "catch load".  */
 
          /* Reinsert breakpoints and continue.  */
-         if (breakpoints_were_inserted)
-           {
-             insert_breakpoints ();
-             breakpoints_inserted = 1;
-           }
+         insert_breakpoints ();
        }
 
       /* If we are skipping through a shell, or through shared library
@@ -1671,7 +1654,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
+      if (regular_breakpoint_inserted_here_p (stop_pc))
        {
          ecs->random_signal = 0;
          if (!breakpoint_thread_match (stop_pc, ecs->ptid))
@@ -1784,7 +1767,6 @@ handle_inferior_event (struct execution_control_state *ecs)
            }
          else
            {                   /* Single step */
-             breakpoints_inserted = 0;
              if (!ptid_equal (inferior_ptid, ecs->ptid))
                context_switch (ecs);
              ecs->waiton_ptid = ecs->ptid;
@@ -1946,7 +1928,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      stack.  */
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      || (breakpoints_inserted
+      || (breakpoint_inserted_here_p (stop_pc)
          && (stop_signal == TARGET_SIGNAL_ILL
              || stop_signal == TARGET_SIGNAL_SEGV
              || stop_signal == TARGET_SIGNAL_EMT))
@@ -2077,8 +2059,8 @@ process_event_stop_test:
        stop_signal = TARGET_SIGNAL_0;
 
       if (prev_pc == read_pc ()
-         && !breakpoints_inserted
          && breakpoint_here_p (read_pc ())
+         && !breakpoint_inserted_here_p (read_pc ())
          && step_resume_breakpoint == NULL)
        {
          /* We were just starting a new sequence, attempting to
@@ -2151,7 +2133,6 @@ process_event_stop_test:
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME\n");
        disable_longjmp_breakpoint ();
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        if (!gdbarch_get_longjmp_target_p (current_gdbarch)
            || !gdbarch_get_longjmp_target (current_gdbarch,
                                            get_current_frame (), &jmp_buf_pc))
@@ -2177,7 +2158,6 @@ process_event_stop_test:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_CLEAR_LONGJMP_RESUME\n");
        remove_breakpoints ();
-       breakpoints_inserted = 0;
        disable_longjmp_breakpoint ();
        ecs->handling_longjmp = 0;      /* FIXME */
        if (what.main_action == BPSTAT_WHAT_CLEAR_LONGJMP_RESUME)
@@ -2187,9 +2167,7 @@ process_event_stop_test:
       case BPSTAT_WHAT_SINGLE:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
-       if (breakpoints_inserted)
-         remove_breakpoints ();
-       breakpoints_inserted = 0;
+       remove_breakpoints ();
        ecs->another_trap = 1;
        /* Still need to check other stuff, at least the case
           where we are stepping and step out of the right range.  */
@@ -2251,7 +2229,6 @@ process_event_stop_test:
               to doing that.  */
            ecs->step_after_step_resume_breakpoint = 0;
            remove_breakpoints ();
-           breakpoints_inserted = 0;
            ecs->another_trap = 1;
            keep_going (ecs);
            return;
@@ -2266,9 +2243,7 @@ process_event_stop_test:
          /* Remove breakpoints, we eventually want to step over the
             shlib event breakpoint, and SOLIB_ADD might adjust
             breakpoint addresses via breakpoint_re_set.  */
-         if (breakpoints_inserted)
-           remove_breakpoints ();
-         breakpoints_inserted = 0;
+         remove_breakpoints ();
 
          /* Check for any newly added shared libraries if we're
             supposed to be adding them automatically.  Switch
@@ -2871,8 +2846,6 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id,
                                                     bp_step_resume);
-  if (breakpoints_inserted)
-    insert_breakpoints ();
 }
 
 /* Insert a "step-resume breakpoint" at RETURN_FRAME.pc.  This is used
@@ -2969,9 +2942,13 @@ keep_going (struct execution_control_state *ecs)
          The signal was SIGTRAP, e.g. it was our signal, but we
          decided we should resume from it.
 
-         We're going to run this baby now!  */
+         We're going to run this baby now!  
 
-      if (!breakpoints_inserted && !ecs->another_trap)
+        Note that insert_breakpoints won't try to re-insert
+        already inserted breakpoints.  Therefore, we don't
+        care if breakpoints were already inserted, or not.  */
+      
+      if (!ecs->another_trap)
        {
          /* Stop stepping when inserting breakpoints
             has failed.  */
@@ -2980,7 +2957,6 @@ keep_going (struct execution_control_state *ecs)
              stop_stepping (ecs);
              return;
            }
-         breakpoints_inserted = 1;
        }
 
       trap_expected = ecs->another_trap;
@@ -3173,7 +3149,7 @@ normal_stop (void)
        gdbarch_decr_pc_after_break needs to just go away.  */
     deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());
 
-  if (target_has_execution && breakpoints_inserted)
+  if (target_has_execution)
     {
       if (remove_breakpoints ())
        {
@@ -3184,7 +3160,6 @@ It might be running in another process.\n\
 Further execution is probably impossible.\n"));
        }
     }
-  breakpoints_inserted = 0;
 
   /* Delete the breakpoint we stopped at, if it wants to be deleted.
      Delete any breakpoint that is to be deleted at the next stop.  */
-- 
1.5.3.5

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Ulrich Weigand]

Vladimir Prus wrote:

> The second problem was that breakpoint_inserted_here_p checks for
> both ordinary breakpoints and single step breakpoints (which don't
> go on bp_location_chain). In most cases, it does not matter, but
> in one place the code tries to figure if we're stopped on a single
> step breakpoint or an ordinary one, and surely, breakpoint_inserted_here_p
> returning 1 for both cases was not good.
> 
> The attached revision of the patch introduces a separate
> regular_breakpoint_inserted_here_p function. No regressions on
> either x86 or arm-linux. OK?

I've tested this on powerpc-ibm-aix5.3.0.0 as well, no regressions.

>         * breakpoint.h (regular_breakpoint_inserted_here_p): New
>         declaration.
>         * breakpoint.c (regular_breakpoint_inserted_here_p): New.
>         (breakpoint_inserted_here_p): Use
>         regular_breakpoint_inserted_here_p.
>         * infrun.c (breakpoints_inserted): Remove.
>         (resume): Don't check for breakpoints_inserted before
>         remove_hw_watchpoints. Use breakpoint_inserted_here_p.
>         (proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
>         (handle_inferior_event): Don't use breakpoints_inserted.
>         Use breakpoints_meant_to_be_inserted and
>         breakpoints_inserted_here_p.
>         (insert_step_resume_breakpoint_at_sal, keep_going): Use
>         breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
>         (normal_stop): Don't check for breakpoints_inserted.  Don't
>         set breakpoints_inserted.
>         (keep_going): Don't check for breakpoints_inserted.
>         (insert_step_resume_breakpoint_at_sal): Don't insert
>         breakpoints

This is OK.

>           /* Remove breakpoints, SOLIB_ADD might adjust
>              breakpoint addresses via breakpoint_re_set.  */
> -         breakpoints_were_inserted = breakpoints_inserted;

"breakpoints_were_inserted" is now unused, could you please remove
the variable definition as well?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Stop infrun from tracking breakpoint insertion status.

[Vladimir Prus]

On Thursday 29 November 2007 01:24:37 Ulrich Weigand wrote:
> Vladimir Prus wrote:
> 
> > The second problem was that breakpoint_inserted_here_p checks for
> > both ordinary breakpoints and single step breakpoints (which don't
> > go on bp_location_chain). In most cases, it does not matter, but
> > in one place the code tries to figure if we're stopped on a single
> > step breakpoint or an ordinary one, and surely, breakpoint_inserted_here_p
> > returning 1 for both cases was not good.
> > 
> > The attached revision of the patch introduces a separate
> > regular_breakpoint_inserted_here_p function. No regressions on
> > either x86 or arm-linux. OK?
> 
> I've tested this on powerpc-ibm-aix5.3.0.0 as well, no regressions.
> 
> >         * breakpoint.h (regular_breakpoint_inserted_here_p): New
> >         declaration.
> >         * breakpoint.c (regular_breakpoint_inserted_here_p): New.
> >         (breakpoint_inserted_here_p): Use
> >         regular_breakpoint_inserted_here_p.
> >         * infrun.c (breakpoints_inserted): Remove.
> >         (resume): Don't check for breakpoints_inserted before
> >         remove_hw_watchpoints. Use breakpoint_inserted_here_p.
> >         (proceed, init_wait_for_inferior): Don't set breakpoints_inserted.
> >         (handle_inferior_event): Don't use breakpoints_inserted.
> >         Use breakpoints_meant_to_be_inserted and
> >         breakpoints_inserted_here_p.
> >         (insert_step_resume_breakpoint_at_sal, keep_going): Use
> >         breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted.
> >         (normal_stop): Don't check for breakpoints_inserted.  Don't
> >         set breakpoints_inserted.
> >         (keep_going): Don't check for breakpoints_inserted.
> >         (insert_step_resume_breakpoint_at_sal): Don't insert
> >         breakpoints
> 
> This is OK.
> 
> >           /* Remove breakpoints, SOLIB_ADD might adjust
> >              breakpoint addresses via breakpoint_re_set.  */
> > -         breakpoints_were_inserted = breakpoints_inserted;
> 
> "breakpoints_were_inserted" is now unused, could you please remove
> the variable definition as well?

I've made this adjustment and checked in. Thanks a lot for review, testing,
and the suggested improvements!

- Volodya