Re: [patch/rfc] Use insert_step_resume_breakpoint everywhere

Re: [patch/rfc] Use insert_step_resume_breakpoint everywhere

[Andrew Cagney]

Hello,

More sigtramp fallout.  This patch eliminates step_over_function, instead using insert_step_resume_breakpoint (get_prev_frame ()).  This way we're closer to a single function that manipulates the step-resume global (few cases still need to be deleted).

When testing this I found a bug in the asser vis:

+  /* Remember, if the call instruction is the last in the step range,
+     the breakpoint will land just beyond that.  Hence ``<=
+     step_range_end''.  */
   gdb_assert (get_frame_pc (step_frame) >= step_range_start
-             && get_frame_pc (step_frame) < step_range_end);
+             && get_frame_pc (step_frame) <= step_range_end);
Further testing revealed an additional bug in this assertion (next vs 
nexti) vis:

+  /* Remember, if the call instruction is the last in the step range,
+     the breakpoint will land just beyond that.  Hence ``<=
+     step_range_end''.  Also, ignore check when "nexti".  */
+  gdb_assert (step_range_start == step_range_end
+             || (get_frame_pc (step_frame) >= step_range_start
+                 && get_frame_pc (step_frame) <= step_range_end));
So I've tweaked that and checked it in.

Andrew

2004-05-10  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (step_over_function): Delete function.
	(handle_step_into_function): Use insert_step_resume_breakpoint.
	(insert_step_resume_breakpoint): Fix assertion.
2004-05-11  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (step_over_function): Delete function.
	(handle_step_into_function): Use insert_step_resume_breakpoint.
	(insert_step_resume_breakpoint): Fix assertion.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.155
diff -p -u -r1.155 infrun.c
--- infrun.c	11 May 2004 16:57:04 -0000	1.155
+++ infrun.c	11 May 2004 23:10:52 -0000
@@ -968,7 +968,6 @@ static void handle_step_into_function (s
 void handle_inferior_event (struct execution_control_state *ecs);
 
 static void step_into_function (struct execution_control_state *ecs);
-static void step_over_function (struct execution_control_state *ecs);
 static void insert_step_resume_breakpoint (struct frame_info *step_frame,
 					   struct execution_control_state *ecs);
 static void stop_stepping (struct execution_control_state *ecs);
@@ -1204,8 +1203,10 @@ handle_step_into_function (struct execut
 
   if (step_over_calls == STEP_OVER_ALL || IGNORE_HELPER_CALL (stop_pc))
     {
-      /* We're doing a "next".  */
-      step_over_function (ecs);
+      /* We're doing a "next", set a breakpoint at callee's return
+	 address (the address at which the caller will resume).  */
+      insert_step_resume_breakpoint (get_prev_frame (get_current_frame ()),
+				     ecs);
       keep_going (ecs);
       return;
     }
@@ -1249,7 +1250,9 @@ handle_step_into_function (struct execut
       return;
     }
 
-  step_over_function (ecs);
+  /* Set a breakpoint at callee's return address (the address at which
+     the caller will resume).  */
+  insert_step_resume_breakpoint (get_prev_frame (get_current_frame ()), ecs);
   keep_going (ecs);
   return;
 }
@@ -2662,8 +2665,12 @@ insert_step_resume_breakpoint (struct fr
   /* This is only used within the step-resume range/frame.  */
   gdb_assert (frame_id_eq (step_frame_id, get_frame_id (step_frame)));
   gdb_assert (step_range_end != 0);
-  gdb_assert (get_frame_pc (step_frame) >= step_range_start
-	      && get_frame_pc (step_frame) < step_range_end);
+  /* Remember, if the call instruction is the last in the step range,
+     the breakpoint will land just beyond that.  Hence ``<=
+     step_range_end''.  Also, ignore check when "nexti".  */
+  gdb_assert (step_range_start == step_range_end
+	      || (get_frame_pc (step_frame) >= step_range_start
+		  && get_frame_pc (step_frame) <= step_range_end));
 
   init_sal (&sr_sal);		/* initialize to zeros */
 
@@ -2675,83 +2682,6 @@ insert_step_resume_breakpoint (struct fr
   step_resume_breakpoint
     = set_momentary_breakpoint (sr_sal, get_frame_id (step_frame),
 				bp_step_resume);
-
-  if (breakpoints_inserted)
-    insert_breakpoints ();
-}
-
-/* We've just entered a callee, and we wish to resume until it returns
-   to the caller.  Setting a step_resume breakpoint on the return
-   address will catch a return from the callee.
-     
-   However, if the callee is recursing, we want to be careful not to
-   catch returns of those recursive calls, but only of THIS instance
-   of the caller.
-
-   To do this, we set the step_resume bp's frame to our current
-   caller's frame (obtained by doing a frame ID unwind).  */
-
-static void
-step_over_function (struct execution_control_state *ecs)
-{
-  struct symtab_and_line sr_sal;
-  struct frame_id sr_id;
-
-  init_sal (&sr_sal);		/* initialize to zeros */
-
-  /* NOTE: cagney/2003-04-06:
-
-     At this point the equality get_frame_pc() == get_frame_func()
-     should hold.  This may make it possible for this code to tell the
-     frame where it's function is, instead of the reverse.  This would
-     avoid the need to search for the frame's function, which can get
-     very messy when there is no debug info available (look at the
-     heuristic find pc start code found in targets like the MIPS).  */
-
-  /* NOTE: cagney/2003-04-06:
-
-     The intent of DEPRECATED_SAVED_PC_AFTER_CALL was to:
-
-     - provide a very light weight equivalent to frame_unwind_pc()
-     (nee FRAME_SAVED_PC) that avoids the prologue analyzer
-
-     - avoid handling the case where the PC hasn't been saved in the
-     prologue analyzer
-
-     Unfortunately, not five lines further down, is a call to
-     get_frame_id() and that is guarenteed to trigger the prologue
-     analyzer.
-     
-     The `correct fix' is for the prologe analyzer to handle the case
-     where the prologue is incomplete (PC in prologue) and,
-     consequently, the return pc has not yet been saved.  It should be
-     noted that the prologue analyzer needs to handle this case
-     anyway: frameless leaf functions that don't save the return PC;
-     single stepping through a prologue.
-
-     The d10v handles all this by bailing out of the prologue analsis
-     when it reaches the current instruction.  */
-
-  if (DEPRECATED_SAVED_PC_AFTER_CALL_P ())
-    sr_sal.pc = ADDR_BITS_REMOVE (DEPRECATED_SAVED_PC_AFTER_CALL (get_current_frame ()));
-  else
-    sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (get_current_frame ()));
-  sr_sal.section = find_pc_overlay (sr_sal.pc);
-
-  check_for_old_step_resume_breakpoint ();
-
-  /* NOTE: cagney/2004-03-31: Code using the current value of
-     "step_frame_id", instead of unwinding that frame ID, removed.  On
-     s390 GNU/Linux, after taking a signal, the program is directly
-     resumed at the signal handler and, consequently, the PC would
-     point at at the first instruction of that signal handler but
-     STEP_FRAME_ID would [incorrectly] at the interrupted code when it
-     should point at the signal trampoline.  By always and locally
-     doing a frame ID unwind, it's possible to assert that the code is
-     always using the correct ID.  */
-  sr_id = frame_unwind_id (get_current_frame ());
-
-  step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id, bp_step_resume);
 
   if (breakpoints_inserted)
     insert_breakpoints ();
From cagney@gnu.org Tue May 11 23:34:00 2004
From: Andrew Cagney <cagney@gnu.org>
To: Nick Roberts <nick@nick.uklinux.net>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [PATCH] Pending breakpoints
Date: Tue, 11 May 2004 23:34:00 -0000
Message-id: <40A162F5.8070601@gnu.org>
References: <16543.59280.786760.919884@nick.uklinux.net> <40A0F1C0.6090205@gnu.org> <16545.10303.526588.598463@nick.uklinux.net> <40A15558.8090803@gnu.org> <16545.24079.489553.335457@nick.uklinux.net>
X-SW-Source: 2004-05/msg00353.html
Content-length: 496

 > >  > Also ok for the branch 
 > > 
 > > For the 6.1.1 respin right? How do I do that?
 > 
 > Yes.  Off the top of my head:
 > 
 > $ cvs -d:ext:sources.redhat.com:/cvs/src co -r gdb_6_1-branch gdb
 > ...

And download millions of files? I'm not falling for that again!
I did:
cvs up -r gdb_6_1-branch
Yes, good point.  For bonus points:
$ tar cf src | ( cd newdir && tar xpf - && cd src && cvs update -r )
It appears to be in, thanks.

Andrew

You might like to check I've not goofed.

Nick

Re: [patch/rfc] Use insert_step_resume_breakpoint everywhere

[Mark Kettenis]

   Date: Tue, 11 May 2004 19:29:56 -0400
   From: Andrew Cagney <cagney@gnu.org>

   Further testing revealed an additional bug in this assertion (next vs 
   nexti) vis:

   +  /* Remember, if the call instruction is the last in the step range,
   +     the breakpoint will land just beyond that.  Hence ``<=
   +     step_range_end''.  Also, ignore check when "nexti".  */
   +  gdb_assert (step_range_start == step_range_end
   +             || (get_frame_pc (step_frame) >= step_range_start
   +                 && get_frame_pc (step_frame) <= step_range_end));

   So I've tweaked that and checked it in.

   Andrew

   > 2004-05-10  Andrew Cagney  <cagney@redhat.com>
   > 
   > 	* infrun.c (step_over_function): Delete function.
   > 	(handle_step_into_function): Use insert_step_resume_breakpoint.
   > 	(insert_step_resume_breakpoint): Fix assertion.
   > 

This patch, or one of your other patches breaks FreeBSD signal
trampoline handling.  I now get:

32      } /* handler */
1: x/i $pc  0x8048545 <handler+13>:     leave  
(gdb) step
../../src/gdb/infrun.c:2672: internal-error: insert_step_resume_breakpoint: Asse
rtion `frame_id_eq (step_frame_id, get_frame_id (step_frame))' failed.
A problem internal to GDB has been detected,
FAIL: gdb.base/siginfo.exp: step out of handler (GDB internal error)

when running the testsuite.  Looks like we're setting a step-resume
breakpoint when one is already set.  The backtrace at this point is:

#0  internal_error (file=0x81b9f76 "../../src/gdb/infrun.c", line=2672, 
    string=0x81b9ff5 "%s: Assertion `%s' failed.") at ../../src/gdb/utils.c:836
#1  0x080b6e9a in insert_step_resume_breakpoint (step_frame=0x825e0fc, 
    ecs=0xbfbfef5c) at ../../src/gdb/infrun.c:2672
#2  0x080b5783 in handle_step_into_function (ecs=0xbfbfef5c)
    at ../../src/gdb/infrun.c:1253
#3  0x080b6a00 in handle_inferior_event (ecs=0xbfbfef5c)
    at ../../src/gdb/infrun.c:2420
#4  0x080b52e0 in wait_for_inferior () at ../../src/gdb/infrun.c:1022
#5  0x080b515b in proceed (addr=4294967295, siggnal=TARGET_SIGNAL_DEFAULT, 
    step=1) at ../../src/gdb/infrun.c:841
#6  0x080b2956 in step_1 (skip_subroutines=0, single_inst=0, count_string=0x0)
    at ../../src/gdb/infcmd.c:661
#7  0x080b26ed in step_command (count_string=0x0, from_tty=1)
    at ../../src/gdb/infcmd.c:549

The value of step_frame_id indicates that we were stepping in handler,
and then try to set a step-resume breakpoint in main.  It might be
relevant to note that on FreeBSD, the signal trampoline calls the
handler, and that signal trampoline is provided by the kernel so we
don't have debug info for it.

Mark

Re: [patch/rfc] Use insert_step_resume_breakpoint everywhere

[Andrew Cagney]

This patch, or one of your other patches breaks FreeBSD signal
trampoline handling.  I now get:
(Which exact FreeBSD)

The value of step_frame_id indicates that we were stepping in handler,
and then try to set a step-resume breakpoint in main.  It might be
relevant to note that on FreeBSD, the signal trampoline calls the
handler, and that signal trampoline is provided by the kernel so we
don't have debug info for it.
If you stepi into the trampoline, does gdb know that its got a signal 
trampoline?  When stepping, this code:

  if (step_range_end != 1
      && (step_over_calls == STEP_OVER_UNDEBUGGABLE
          || step_over_calls == STEP_OVER_ALL)
      && get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME)
    {
      /* The inferior, while doing a "step" or "next", has ended up in
         a signal trampoline (either by a signal being delivered or by
         the signal handler returning).  Just single-step until the
         inferior leaves the trampoline (either by calling the handler
         or returning).  */
      keep_going (ecs);
      return;
    }
should be triggering causing the inferior to single step which makes it 
setting a breakpoint in main most puzzling.

Try moving the above to before:

  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
      && ecs->stop_func_name == NULL)
    {
      /* There is no symbol, not even a minimal symbol, corresponding
         to the address where we just stopped.  So we just stepped
         inside undebuggable code.  Since we want to step over this
         kind of code, we keep going until the inferior returns from
         the current function.  */
      handle_step_into_function (ecs);
      return;
    }
Andrew

[commit] Check undebuggable after sigtramp; Was: [patch/rfc] Useinsert_step_resume_breakpoint everywhere

[Andrew Cagney]

If you stepi into the trampoline, does gdb know that its got a signal trampoline?  When stepping, this code:

  if (step_range_end != 1
      && (step_over_calls == STEP_OVER_UNDEBUGGABLE
          || step_over_calls == STEP_OVER_ALL)
      && get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME)
    {
      /* The inferior, while doing a "step" or "next", has ended up in
         a signal trampoline (either by a signal being delivered or by
         the signal handler returning).  Just single-step until the
         inferior leaves the trampoline (either by calling the handler
         or returning).  */
      keep_going (ecs);
      return;
    }
should be triggering causing the inferior to single step which makes it setting a breakpoint in main most puzzling.

Try moving the above to before:

  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
      && ecs->stop_func_name == NULL)
    {
      /* There is no symbol, not even a minimal symbol, corresponding
         to the address where we just stopped.  So we just stepped
         inside undebuggable code.  Since we want to step over this
         kind of code, we keep going until the inferior returns from
         the current function.  */
      handle_step_into_function (ecs);
      return;
    }
Per the attached I've done this, I'm no longer seeing the panic.

committed,
Andrew
2004-05-13  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (handle_inferior_event): Check for
	STEP_OVER_UNDEBUGGABLE after signal trampolines and function
	calls.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.160
diff -p -u -r1.160 infrun.c
--- infrun.c	13 May 2004 18:42:29 -0000	1.160
+++ infrun.c	13 May 2004 19:25:13 -0000
@@ -2411,18 +2411,6 @@ process_event_stop_test:
       return;
     }
 
-  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
-      && ecs->stop_func_name == NULL)
-    {
-      /* There is no symbol, not even a minimal symbol, corresponding
-         to the address where we just stopped.  So we just stepped
-         inside undebuggable code.  Since we want to step over this
-         kind of code, we keep going until the inferior returns from
-         the current function.  */
-      handle_step_into_function (ecs);
-      return;
-    }
-
   if (step_range_end != 1
       && (step_over_calls == STEP_OVER_UNDEBUGGABLE
 	  || step_over_calls == STEP_OVER_ALL)
@@ -2434,6 +2422,22 @@ process_event_stop_test:
 	 inferior leaves the trampoline (either by calling the handler
 	 or returning).  */
       keep_going (ecs);
+      return;
+    }
+
+  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
+      && ecs->stop_func_name == NULL)
+    {
+      /* The inferior just stepped into, or returned to, an
+         undebuggable function (where there is no symbol, not even a
+         minimal symbol, corresponding to the address where the
+         inferior stopped).  Since we want to skip this kind of code,
+         we keep going until the inferior returns from this
+         function.  */
+      /* NOTE: cagney/2004-05-12: This test is performed after the
+	 sigtramp test as often sigtramps, while recognized by GDB,
+	 have no symbol information.  */
+      handle_step_into_function (ecs);
       return;
     }
 
From kettenis@chello.nl Thu May 13 19:52:00 2004
From: Mark Kettenis <kettenis@chello.nl>
To: cagney@gnu.org
Cc: gdb-patches@sources.redhat.com
Subject: Re: [commit] Check undebuggable after sigtramp; Was: [patch/rfc] Useinsert_step_resume_breakpoint everywhere
Date: Thu, 13 May 2004 19:52:00 -0000
Message-id: <200405131952.i4DJqhxV019690@elgar.kettenis.dyndns.org>
References: <40A01953.7060906@gnu.org> <40A161F4.6050801@gnu.org> <200405122029.i4CKT0r4006056@elgar.kettenis.dyndns.org> <40A28D7B.90007@gnu.org> <40A3CF09.70807@gnu.org>
X-SW-Source: 2004-05/msg00414.html
Content-length: 250

   Date: Thu, 13 May 2004 15:39:53 -0400
   From: Andrew Cagney <cagney@gnu.org>

   Per the attached I've done this, I'm no longer seeing the panic.

Works for me too.  By the way, it was FreeBSD 4.7-RELEASE-p20 that was
showing the problem.

Mark