This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.  */


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]