This is the mail archive of the gdb-patches@sources.redhat.com 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]

[commit] Don't use STEP_FRAME_ID; Was: [PATCH] Fix signals.exp testcase on S/390


I've committted the attached.

As Ulrich explains in the below, trying to use STEP_FRAME_ID was creating a big nasty rat hole.
The patch does preserve the old behavior when legacy_frame_p (hmm, which must be on its last legs :-).


Andrew Cagney wrote:

> It will run into the first if, and simply use step_frame_id,
> which is wrong in this case.  That's why my patch add another
> condition to the first if, to make it not taken and actually
> use the (correct) get_prev_frame case.


Where is step_frame_id pointing?


To the function that was interrupted by the signal (i.e. the
function where I entered 'next').

Good.


Anyway, I think this code:
> if (frame_id_p (step_frame_id)
> && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc))
> /* NOTE: cagney/2004-02-27: Use the global state's idea of the
> stepping frame ID. I suspect this is done as it is lighter
> weight than a call to get_prev_frame. */
> sr_id = step_frame_id;
should simply be deleted. I wondered about it and you've just confirmed my suspicions. With that code gone is half the problem solved?


Yes, deleting this works just fine for me, in fact ...

Good.


That leaves the other problem, which is much harder :-(


... it even solves the other problem as well!

Yow!


The reason for this is that the whole problematic if that uses frame_id_inner becomes irrelevant:

      if (pc_in_sigtramp (stop_pc)
          && frame_id_inner (step_frame_id,
                             frame_id_build (read_sp (), 0)))
        /* We stepped out of a signal handler, and into its
           calling trampoline.  This is misdetected as a
           subroutine call, but stepping over the signal
           trampoline isn't such a bad idea.  In order to do that,
           we have to ignore the value in step_frame_id, since
           that doesn't represent the frame that'll reach when we
           return from the signal trampoline.  Otherwise we'll
           probably continue to the end of the program.  */
        step_frame_id = null_frame_id;

step_over_function (ecs);

With those lines in step_over_function deleted, step_over_function
does not care about step_frame_id at all any more, and thus there
is no need to fiddle with step_frame_id here ...

> Finally, the patch below reintroduces a pc_in_sigtramp
> gdbarch callback to s390-tdep.c; I had thought this would
> be no longer necessary when using the new frame code, but
> apparently there's still other users ...


Yes, it shouldn't be needed. get_frame_type == SIGTRAMP_FRAME is sufficient. work-in-progress.


Actually, when deleting the lines in step_over_function, it turns
out that I don't need pc_in_sigtramp any more ...

Summing up: after completely reverting my patch, and simply deleting those lines, I get a gdb that passes signals.exp
(and has no test suite regressions), and also handles stepping
out of a signal handler correctly.

ya!


thanks,
Andrew

Index: ChangeLog
2004-03-15  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (handle_step_into_function, step_over_function): Only
	update and use STEP_FRAME_ID when the system is using legacy
	frames.  Update comments.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.139
diff -u -r1.139 infrun.c
--- infrun.c	9 Mar 2004 16:40:08 -0000	1.139
+++ infrun.c	15 Mar 2004 16:58:35 -0000
@@ -1264,17 +1264,22 @@
     {
       /* We're doing a "next".  */
 
-      if (pc_in_sigtramp (stop_pc)
+      if (legacy_frame_p (current_gdbarch)
+	  && pc_in_sigtramp (stop_pc)
           && frame_id_inner (step_frame_id,
                              frame_id_build (read_sp (), 0)))
-        /* We stepped out of a signal handler, and into its
-           calling trampoline.  This is misdetected as a
-           subroutine call, but stepping over the signal
-           trampoline isn't such a bad idea.  In order to do that,
-           we have to ignore the value in step_frame_id, since
-           that doesn't represent the frame that'll reach when we
-           return from the signal trampoline.  Otherwise we'll
-           probably continue to the end of the program.  */
+	/* NOTE: cagney/2004-03-15: This is only needed for legacy
+	   systems.  On non-legacy systems step_over_function doesn't
+	   use STEP_FRAME_ID and hence the below update "hack" isn't
+	   needed.  */
+        /* We stepped out of a signal handler, and into its calling
+           trampoline.  This is misdetected as a subroutine call, but
+           stepping over the signal trampoline isn't such a bad idea.
+           In order to do that, we have to ignore the value in
+           step_frame_id, since that doesn't represent the frame
+           that'll reach when we return from the signal trampoline.
+           Otherwise we'll probably continue to the end of the
+           program.  */
         step_frame_id = null_frame_id;
 
       step_over_function (ecs);
@@ -2868,11 +2873,10 @@
      
    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 call.
+   of the caller.
 
    To do this, we set the step_resume bp's frame to our current
-   caller's frame (step_frame_id, which is set by the "next" or
-   "until" command, before execution begins).  */
+   caller's frame (obtained by doing a frame ID unwind).  */
 
 static void
 step_over_function (struct execution_control_state *ecs)
@@ -2923,24 +2927,40 @@
 
   check_for_old_step_resume_breakpoint ();
 
-  if (frame_id_p (step_frame_id)
-      && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc))
-    /* NOTE: cagney/2004-02-27: Use the global state's idea of the
-       stepping frame ID.  I suspect this is done as it is lighter
-       weight than a call to get_prev_frame.  */
-    sr_id = step_frame_id;
-  else if (legacy_frame_p (current_gdbarch))
-    /* NOTE: cagney/2004-02-27: This is the way it was 'cos this is
-       the way it always was.  It should be using the unwound (or
-       caller's) ID, and not this (or the callee's) ID.  It appeared
-       to work because: legacy architectures used the wrong end of the
-       frame for the ID.stack (inner-most rather than outer-most) so
-       that the callee's id.stack (un adjusted) matched the caller's
-       id.stack giving the "correct" id; more often than not
-       !IN_SOLIB_DYNSYM_RESOLVE_CODE and hence the code above (it was
-       originally later in the function) fixed the ID by using global
-       state.  */
-    sr_id = get_frame_id (get_current_frame ());
+  /* NOTE: cagney/2004-03-15: Code using the current value of
+     "step_frame_id", instead of unwinding that frame ID, removed (at
+     least for non-legacy platforms).  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.  */
+  if (legacy_frame_p (current_gdbarch))
+    {
+      if (frame_id_p (step_frame_id)
+	  && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc))
+	/* NOTE: cagney/2004-02-27: Use the global state's idea of the
+	   stepping frame ID.  I suspect this is done as it is lighter
+	   weight than a call to get_prev_frame.  */
+	/* NOTE: cagney/2004-03-15: See comment above about how this
+	   is also broken.  */
+	sr_id = step_frame_id;
+      else
+	/* NOTE: cagney/2004-03-15: This is the way it was 'cos this
+	   is the way it always was.  It should be using the unwound
+	   (or caller's) ID, and not this (or the callee's) ID.  It
+	   appeared to work because: legacy architectures used the
+	   wrong end of the frame for the ID.stack (inner-most rather
+	   than outer-most) so that the callee's id.stack (un
+	   adjusted) matched the caller's id.stack giving the
+	   "correct" id; more often than not
+	   !IN_SOLIB_DYNSYM_RESOLVE_CODE and hence the code above (it
+	   was originally later in the function) fixed the ID by using
+	   global state.  */
+	sr_id = get_frame_id (get_current_frame ());
+    }
   else
     sr_id = get_frame_id (get_prev_frame (get_current_frame ()));
 

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