Re: [PATCH] Fix signals.exp test case on S/390

Re: [PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

Andrew Cagney wrote:

>I'm lost here. What happens with:
> 
>- get_frame_id (get_prev_frame (signal handler))
>- get_frame_id (sigreturn trampoline)
> 
>They should match

Well, they do match, that's why things work with my patch.

The problem is that without my patch, step_over_function
doesn't actually *use* get_frame_id (get_prev_frame ...),
see the 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;
  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 ());
  else
    sr_id = get_frame_id (get_prev_frame (get_current_frame ()));

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.
 
Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [PATCH] Fix signals.exp test case on S/390

[Andrew Cagney]

> Andrew Cagney wrote:
> 
> 
>>>I'm lost here. What happens with:
>>> 
>>>- get_frame_id (get_prev_frame (signal handler))
>>>- get_frame_id (sigreturn trampoline)
>>> 
>>>They should match
> 
> 
> Well, they do match, that's why things work with my patch.

Ok, good.

> The problem is that without my patch, step_over_function
> doesn't actually *use* get_frame_id (get_prev_frame ...),
> see the 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;

>   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 ());
>   else
>     sr_id = get_frame_id (get_prev_frame (get_current_frame ()));
> 
> 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?

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?

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

> *************** handle_step_into_function (struct execut
> *** 1265,1272 ****
>         /* We're doing a "next".  */
>   
>         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
> --- 1265,1271 ----
>         /* We're doing a "next".  */
>   
>         if (pc_in_sigtramp (stop_pc)
> !           && INNER_THAN (step_sp, read_sp ()))
>           /* We stepped out of a signal handler, and into its
>              calling trampoline.  This is misdetected as a
>              subroutine call, but stepping over the signal

Both INNER_THAN and frame_id_build(read_sp (),0) / frame_id_inner are 
wrong here. They test variations on:

/* Returns non-zero when L is strictly inner-than R (they have
    different frame .bases).  Neither L, nor R can be `null'.  See note
    above about frameless functions.  */
...
/* Methods for constructing and comparing Frame IDs.

    NOTE: Given stackless functions A and B, where A calls B (and hence
    B is inner-to A).  The relationships: !eq(A,B); !eq(B,A);
    !inner(A,B); !inner(B,A); all hold.

    This is because, while B is inner-to A, B is not strictly inner-to A.
    Being stackless, they have an identical .stack_addr value, and differ
    only by their unordered .code_addr and/or .special_addr values.

    Because frame_id_inner is only used as a safety net (e.g.,
    detect a corrupt stack) the lack of strictness is not a problem.
    Code needing to determine an exact relationship between two frames
    must instead use frame_id_eq and frame_id_unwind.  For instance,
    in the above, to determine that A stepped-into B, the equation
    "A.id != B.id && A.id == id_unwind (B)" can be used.  */

and that isn't sufficient.  It's easy to construct a situtation where 
the handler and sigtramp have the same ID/SP.

A better strategy here might be to, if the user isn't instruction 
stepping or cntrl-ced, single-step the inferior until it finds its way 
out of the sigtramp and then make a decision.

Andrew

PS:

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

Re: [PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

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').

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

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

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

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.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

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

[Andrew Cagney]

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

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


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5500 bytes --]

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 ()));
 

Re: [commit] Don't use STEP_FRAME_ID; Was: [PATCH] Fix signals.exp test

[Ulrich Weigand]

Andrew Cagney wrote:

> I've committted the attached.

Thanks; this fixes the signals.exp failures on s390*.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

Andrew Cagney wrote:

>I'm lost here. What happens with:
> 
>- get_frame_id (get_prev_frame (signal handler))
>- get_frame_id (sigreturn trampoline)
> 
>They should match

Well, they do match, that's why things work with my patch.

The problem is that without my patch, step_over_function
doesn't actually *use* get_frame_id (get_prev_frame ...),
see the 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;
  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 ());
  else
    sr_id = get_frame_id (get_prev_frame (get_current_frame ()));

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.
 
Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

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').

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

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

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

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.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [commit] Don't use STEP_FRAME_ID; Was: [PATCH] Fix signals.exp test

[Ulrich Weigand]

Andrew Cagney wrote:

> I've committted the attached.

Thanks; this fixes the signals.exp failures on s390*.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [PATCH] Fix signals.exp test case on S/390

[Andrew Cagney]

> Andrew Cagney wrote:
> 
> 
>>>I'm lost here. What happens with:
>>> 
>>>- get_frame_id (get_prev_frame (signal handler))
>>>- get_frame_id (sigreturn trampoline)
>>> 
>>>They should match
> 
> 
> Well, they do match, that's why things work with my patch.

Ok, good.

> The problem is that without my patch, step_over_function
> doesn't actually *use* get_frame_id (get_prev_frame ...),
> see the 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;

>   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 ());
>   else
>     sr_id = get_frame_id (get_prev_frame (get_current_frame ()));
> 
> 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?

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?

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

> *************** handle_step_into_function (struct execut
> *** 1265,1272 ****
>         /* We're doing a "next".  */
>   
>         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
> --- 1265,1271 ----
>         /* We're doing a "next".  */
>   
>         if (pc_in_sigtramp (stop_pc)
> !           && INNER_THAN (step_sp, read_sp ()))
>           /* We stepped out of a signal handler, and into its
>              calling trampoline.  This is misdetected as a
>              subroutine call, but stepping over the signal

Both INNER_THAN and frame_id_build(read_sp (),0) / frame_id_inner are 
wrong here. They test variations on:

/* Returns non-zero when L is strictly inner-than R (they have
    different frame .bases).  Neither L, nor R can be `null'.  See note
    above about frameless functions.  */
...
/* Methods for constructing and comparing Frame IDs.

    NOTE: Given stackless functions A and B, where A calls B (and hence
    B is inner-to A).  The relationships: !eq(A,B); !eq(B,A);
    !inner(A,B); !inner(B,A); all hold.

    This is because, while B is inner-to A, B is not strictly inner-to A.
    Being stackless, they have an identical .stack_addr value, and differ
    only by their unordered .code_addr and/or .special_addr values.

    Because frame_id_inner is only used as a safety net (e.g.,
    detect a corrupt stack) the lack of strictness is not a problem.
    Code needing to determine an exact relationship between two frames
    must instead use frame_id_eq and frame_id_unwind.  For instance,
    in the above, to determine that A stepped-into B, the equation
    "A.id != B.id && A.id == id_unwind (B)" can be used.  */

and that isn't sufficient.  It's easy to construct a situtation where 
the handler and sigtramp have the same ID/SP.

A better strategy here might be to, if the user isn't instruction 
stepping or cntrl-ced, single-step the inferior until it finds its way 
out of the sigtramp and then make a decision.

Andrew

PS:

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

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

[Andrew Cagney]

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

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


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5500 bytes --]

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 ()));
 

Re: [PATCH] Fix signals.exp test case on S/390

[Andrew Cagney]

> Hello,
> 
> I was trying to get the signals.exp test case to pass on s390.
> This exposed a number of problems in the Linux kernel and gdb;
> in particular I think I've found a bug in gdb common code
> (step_over_function in infrun.c).
> 
> However, I'm not completely sure I fully understand the intended
> flow of control throughout infrun.c, so I'd appreciate comments
> on what I found.
> 
> Basically, the test case arranges for a signal (SIGALRM) to
> arrive while the inferior is stopped, and then does a 'next'.
> When the inferior is continued with PT_SINGLESTEP due to the
> 'next', the kernel tries to deliver the pending SIGALRM.
> 
> This in turn causes another ptrace intercept.  gdb gets active,
> notices that it doesn't want to do anything special with the
> signal, and continues with PT_SINGLESTEP giving the signal
> number.
> 
> At this point, the Linux kernel delivers the signal, runs to
> the first instruction of the signal handler, and gets the
> single step trap there.  (Note that there was a bug in the 
> kernel that caused this not to work, but I've fixed that one.)
> 
> Now the interesting aspect starts.  handle_inferior_event
> needs to figure out what has happened here.  There is special
> code in handle_inferior_event that tries to recognize the
> 'we've just stepped into a signal handler' situation:
> 
>   /* Did we just take a signal?  */
>   if (pc_in_sigtramp (stop_pc)
>       && !pc_in_sigtramp (prev_pc)
>       && INNER_THAN (read_sp (), step_sp))

[anything in infrun.c using INNER_THAN is always suspect]

>     {
>       /* We've just taken a signal; go until we are back to
>          the point where we took it and one more.  */
> 
> Note, however, that this fundamentally does not work on
> Linux because we use a signal trampoline only to *exit*
> from a signal handler -- the kernel *starts* signal handlers
> by jumping to them directly without any trampoline.
> 
> So this test doesn't trigger, and the event turns out to
> be interpreted as call to a subroutine, and is passed on
> to handle_step_info_function, which decides to step over
> that function call using step_over_function.  This would 
> basically work just fine for this case -- we'd continue
> just after the signal handler terminates, which is what
> we want here.
> 
> However, step_over_function continues to run not only
> until a specific PC has been reached, but at the same time
> a specific *frame* needs to be reached.  In the situation
> we're in right now:
> 
>   PC                                        frame
>   1st instruction of signal handler         sig. handler frame
>   1st instruction of sigreturn trampoline   sigtramp frame
>   interrupted main routine                  main routine frame
> 
> step_over_function tries to continue running until it
> reaches the current frame's return address (i.e. the 1st
> instruction of the sigreturn trampoline) while at the same
> time reaching the frame currently being stepped (i.e the
> main routine's frame).  Of course, these two events never
> coincide, and hence gdb steps until the program terminates.

I'm lost here.  What happens with:

- get_frame_id (get_prev_frame (signal handler))

- get_frame_id (sigreturn trampoline)

Hopefully you can see the values by grubbing around in the output from 
"set debug frame 1".

They should match (check my tramp branch, it contains, er, interesting 
ideas on how to better handle trampolines).

Andrew

[PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

Hello,

I was trying to get the signals.exp test case to pass on s390.
This exposed a number of problems in the Linux kernel and gdb;
in particular I think I've found a bug in gdb common code
(step_over_function in infrun.c).

However, I'm not completely sure I fully understand the intended
flow of control throughout infrun.c, so I'd appreciate comments
on what I found.

Basically, the test case arranges for a signal (SIGALRM) to
arrive while the inferior is stopped, and then does a 'next'.
When the inferior is continued with PT_SINGLESTEP due to the
'next', the kernel tries to deliver the pending SIGALRM.

This in turn causes another ptrace intercept.  gdb gets active,
notices that it doesn't want to do anything special with the
signal, and continues with PT_SINGLESTEP giving the signal
number.

At this point, the Linux kernel delivers the signal, runs to
the first instruction of the signal handler, and gets the
single step trap there.  (Note that there was a bug in the 
kernel that caused this not to work, but I've fixed that one.)

Now the interesting aspect starts.  handle_inferior_event
needs to figure out what has happened here.  There is special
code in handle_inferior_event that tries to recognize the
'we've just stepped into a signal handler' situation:

  /* Did we just take a signal?  */
  if (pc_in_sigtramp (stop_pc)
      && !pc_in_sigtramp (prev_pc)
      && INNER_THAN (read_sp (), step_sp))
    {
      /* We've just taken a signal; go until we are back to
         the point where we took it and one more.  */

Note, however, that this fundamentally does not work on
Linux because we use a signal trampoline only to *exit*
from a signal handler -- the kernel *starts* signal handlers
by jumping to them directly without any trampoline.

So this test doesn't trigger, and the event turns out to
be interpreted as call to a subroutine, and is passed on
to handle_step_info_function, which decides to step over
that function call using step_over_function.  This would 
basically work just fine for this case -- we'd continue
just after the signal handler terminates, which is what
we want here.

However, step_over_function continues to run not only
until a specific PC has been reached, but at the same time
a specific *frame* needs to be reached.  In the situation
we're in right now:

  PC                                        frame
  1st instruction of signal handler         sig. handler frame
  1st instruction of sigreturn trampoline   sigtramp frame
  interrupted main routine                  main routine frame

step_over_function tries to continue running until it
reaches the current frame's return address (i.e. the 1st
instruction of the sigreturn trampoline) while at the same
time reaching the frame currently being stepped (i.e the
main routine's frame).  Of course, these two events never
coincide, and hence gdb steps until the program terminates.

Now, a similar situation occurs if there is a dynamic linker
resolver frame between a called subroutine and the main 
routine, and there is special code in step_over_function to
skip the intermediate frame in this case.  The patch below
extends this special handling to the case of an intermediate
signal trampoline frame, which fixes the symptom for me --
the signals.exp test case now passes.

Is this an acceptable solution or should the signal-handler
code in handle_inferior_event be made smarter?


Also, I've run into another problem when single-stepping
*out* of a signal handler.  In this case code in 
handle_step_into_function is supposed to recognize that
we've just stepped into a signal trampoline and just
continue on over the sigreturn system call.

This doesn't work on s390 because of the comparison:
          && frame_id_inner (step_frame_id,
                             frame_id_build (read_sp (), 0)))

since our frame IDs contain the DWARF CFA as stack_addr,
which is biased relative to the stack pointer at function
entry.  So comparing this CFA to a real SP value is bogus.

Other places compare either two stack frame IDs or two
real SP values, either of which should work.  The patch below
changes the comparison to 
          && INNER_THAN (step_sp, read_sp ()))

similar to most other places in infrun.c that manage
signal trampolines.


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

The whole patch builds on s390-ibm-linux with no test suite
regressions and fixes the signals.exp test case.

Bye,
Ulrich

ChangeLog:

	* infrun.c (handle_step_into_function): Do not compare a frame ID
	stack_addr directly against a read_sp () stack pointer value.
	(step_over_function): Skip signal trampoline frame on function
	return.
	* s390-tdep.c (s390_sigtramp_frame_sniffer): Move core signal
	trampoline detection logic to ...
	(s390_pc_in_sigtramp): ... this new function.
	(s390_gdbarch_init): Call set_gdbarch_pc_in_sigtramp.


Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.138
diff -c -p -r1.138 infrun.c
*** gdb/infrun.c	6 Mar 2004 00:10:06 -0000	1.138
--- gdb/infrun.c	10 Mar 2004 01:33:06 -0000
*************** handle_step_into_function (struct execut
*** 1265,1272 ****
        /* We're doing a "next".  */
  
        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
--- 1265,1271 ----
        /* We're doing a "next".  */
  
        if (pc_in_sigtramp (stop_pc)
!           && INNER_THAN (step_sp, read_sp ()))
          /* We stepped out of a signal handler, and into its
             calling trampoline.  This is misdetected as a
             subroutine call, but stepping over the signal
*************** step_over_function (struct execution_con
*** 2976,2982 ****
    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.  */
--- 2975,2982 ----
    check_for_old_step_resume_breakpoint ();
  
    if (frame_id_p (step_frame_id)
!       && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc)
!       && !pc_in_sigtramp (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.  */
Index: gdb/s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.129
diff -c -p -r1.129 s390-tdep.c
*** gdb/s390-tdep.c	26 Feb 2004 23:48:01 -0000	1.129
--- gdb/s390-tdep.c	10 Mar 2004 01:33:06 -0000
*************** static const struct frame_unwind s390_si
*** 2237,2258 ****
    s390_sigtramp_frame_prev_register
  };
  
! static const struct frame_unwind *
! s390_sigtramp_frame_sniffer (struct frame_info *next_frame)
  {
-   CORE_ADDR pc = frame_pc_unwind (next_frame);
    bfd_byte sigreturn[2];
- 
    if (read_memory_nobpt (pc, sigreturn, 2))
!     return NULL;
  
    if (sigreturn[0] != 0x0a /* svc */)
!     return NULL;
  
    if (sigreturn[1] != 119 /* sigreturn */
        && sigreturn[1] != 173 /* rt_sigreturn */)
      return NULL;
!   
    return &s390_sigtramp_frame_unwind;
  }
  
--- 2237,2266 ----
    s390_sigtramp_frame_prev_register
  };
  
! static int
! s390_pc_in_sigtramp (CORE_ADDR pc, char *name)
  {
    bfd_byte sigreturn[2];
    if (read_memory_nobpt (pc, sigreturn, 2))
!     return 0;
  
    if (sigreturn[0] != 0x0a /* svc */)
!     return 0;
  
    if (sigreturn[1] != 119 /* sigreturn */
        && sigreturn[1] != 173 /* rt_sigreturn */)
+     return 0;
+ 
+   return 1;
+ }
+ 
+ static const struct frame_unwind *
+ s390_sigtramp_frame_sniffer (struct frame_info *next_frame)
+ {
+   CORE_ADDR pc = frame_pc_unwind (next_frame);
+   if (!s390_pc_in_sigtramp (pc, NULL))
      return NULL;
! 
    return &s390_sigtramp_frame_unwind;
  }
  
*************** s390_gdbarch_init (struct gdbarch_info i
*** 3024,3029 ****
--- 3032,3038 ----
    set_gdbarch_return_value (gdbarch, s390_return_value);
  
    /* Frame handling.  */
+   set_gdbarch_pc_in_sigtramp (gdbarch, s390_pc_in_sigtramp);
    set_gdbarch_in_solib_call_trampoline (gdbarch, in_plt_section);
    dwarf2_frame_set_init_reg (gdbarch, s390_dwarf2_frame_init_reg);
    frame_unwind_append_sniffer (gdbarch, dwarf2_frame_sniffer);
-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [PATCH] Fix signals.exp test case on S/390

[Andrew Cagney]

> Hello,
> 
> I was trying to get the signals.exp test case to pass on s390.
> This exposed a number of problems in the Linux kernel and gdb;
> in particular I think I've found a bug in gdb common code
> (step_over_function in infrun.c).
> 
> However, I'm not completely sure I fully understand the intended
> flow of control throughout infrun.c, so I'd appreciate comments
> on what I found.
> 
> Basically, the test case arranges for a signal (SIGALRM) to
> arrive while the inferior is stopped, and then does a 'next'.
> When the inferior is continued with PT_SINGLESTEP due to the
> 'next', the kernel tries to deliver the pending SIGALRM.
> 
> This in turn causes another ptrace intercept.  gdb gets active,
> notices that it doesn't want to do anything special with the
> signal, and continues with PT_SINGLESTEP giving the signal
> number.
> 
> At this point, the Linux kernel delivers the signal, runs to
> the first instruction of the signal handler, and gets the
> single step trap there.  (Note that there was a bug in the 
> kernel that caused this not to work, but I've fixed that one.)
> 
> Now the interesting aspect starts.  handle_inferior_event
> needs to figure out what has happened here.  There is special
> code in handle_inferior_event that tries to recognize the
> 'we've just stepped into a signal handler' situation:
> 
>   /* Did we just take a signal?  */
>   if (pc_in_sigtramp (stop_pc)
>       && !pc_in_sigtramp (prev_pc)
>       && INNER_THAN (read_sp (), step_sp))

[anything in infrun.c using INNER_THAN is always suspect]

>     {
>       /* We've just taken a signal; go until we are back to
>          the point where we took it and one more.  */
> 
> Note, however, that this fundamentally does not work on
> Linux because we use a signal trampoline only to *exit*
> from a signal handler -- the kernel *starts* signal handlers
> by jumping to them directly without any trampoline.
> 
> So this test doesn't trigger, and the event turns out to
> be interpreted as call to a subroutine, and is passed on
> to handle_step_info_function, which decides to step over
> that function call using step_over_function.  This would 
> basically work just fine for this case -- we'd continue
> just after the signal handler terminates, which is what
> we want here.
> 
> However, step_over_function continues to run not only
> until a specific PC has been reached, but at the same time
> a specific *frame* needs to be reached.  In the situation
> we're in right now:
> 
>   PC                                        frame
>   1st instruction of signal handler         sig. handler frame
>   1st instruction of sigreturn trampoline   sigtramp frame
>   interrupted main routine                  main routine frame
> 
> step_over_function tries to continue running until it
> reaches the current frame's return address (i.e. the 1st
> instruction of the sigreturn trampoline) while at the same
> time reaching the frame currently being stepped (i.e the
> main routine's frame).  Of course, these two events never
> coincide, and hence gdb steps until the program terminates.

I'm lost here.  What happens with:

- get_frame_id (get_prev_frame (signal handler))

- get_frame_id (sigreturn trampoline)

Hopefully you can see the values by grubbing around in the output from 
"set debug frame 1".

They should match (check my tramp branch, it contains, er, interesting 
ideas on how to better handle trampolines).

Andrew

[PATCH] Fix signals.exp test case on S/390

[Ulrich Weigand]

Hello,

I was trying to get the signals.exp test case to pass on s390.
This exposed a number of problems in the Linux kernel and gdb;
in particular I think I've found a bug in gdb common code
(step_over_function in infrun.c).

However, I'm not completely sure I fully understand the intended
flow of control throughout infrun.c, so I'd appreciate comments
on what I found.

Basically, the test case arranges for a signal (SIGALRM) to
arrive while the inferior is stopped, and then does a 'next'.
When the inferior is continued with PT_SINGLESTEP due to the
'next', the kernel tries to deliver the pending SIGALRM.

This in turn causes another ptrace intercept.  gdb gets active,
notices that it doesn't want to do anything special with the
signal, and continues with PT_SINGLESTEP giving the signal
number.

At this point, the Linux kernel delivers the signal, runs to
the first instruction of the signal handler, and gets the
single step trap there.  (Note that there was a bug in the 
kernel that caused this not to work, but I've fixed that one.)

Now the interesting aspect starts.  handle_inferior_event
needs to figure out what has happened here.  There is special
code in handle_inferior_event that tries to recognize the
'we've just stepped into a signal handler' situation:

  /* Did we just take a signal?  */
  if (pc_in_sigtramp (stop_pc)
      && !pc_in_sigtramp (prev_pc)
      && INNER_THAN (read_sp (), step_sp))
    {
      /* We've just taken a signal; go until we are back to
         the point where we took it and one more.  */

Note, however, that this fundamentally does not work on
Linux because we use a signal trampoline only to *exit*
from a signal handler -- the kernel *starts* signal handlers
by jumping to them directly without any trampoline.

So this test doesn't trigger, and the event turns out to
be interpreted as call to a subroutine, and is passed on
to handle_step_info_function, which decides to step over
that function call using step_over_function.  This would 
basically work just fine for this case -- we'd continue
just after the signal handler terminates, which is what
we want here.

However, step_over_function continues to run not only
until a specific PC has been reached, but at the same time
a specific *frame* needs to be reached.  In the situation
we're in right now:

  PC                                        frame
  1st instruction of signal handler         sig. handler frame
  1st instruction of sigreturn trampoline   sigtramp frame
  interrupted main routine                  main routine frame

step_over_function tries to continue running until it
reaches the current frame's return address (i.e. the 1st
instruction of the sigreturn trampoline) while at the same
time reaching the frame currently being stepped (i.e the
main routine's frame).  Of course, these two events never
coincide, and hence gdb steps until the program terminates.

Now, a similar situation occurs if there is a dynamic linker
resolver frame between a called subroutine and the main 
routine, and there is special code in step_over_function to
skip the intermediate frame in this case.  The patch below
extends this special handling to the case of an intermediate
signal trampoline frame, which fixes the symptom for me --
the signals.exp test case now passes.

Is this an acceptable solution or should the signal-handler
code in handle_inferior_event be made smarter?


Also, I've run into another problem when single-stepping
*out* of a signal handler.  In this case code in 
handle_step_into_function is supposed to recognize that
we've just stepped into a signal trampoline and just
continue on over the sigreturn system call.

This doesn't work on s390 because of the comparison:
          && frame_id_inner (step_frame_id,
                             frame_id_build (read_sp (), 0)))

since our frame IDs contain the DWARF CFA as stack_addr,
which is biased relative to the stack pointer at function
entry.  So comparing this CFA to a real SP value is bogus.

Other places compare either two stack frame IDs or two
real SP values, either of which should work.  The patch below
changes the comparison to 
          && INNER_THAN (step_sp, read_sp ()))

similar to most other places in infrun.c that manage
signal trampolines.


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

The whole patch builds on s390-ibm-linux with no test suite
regressions and fixes the signals.exp test case.

Bye,
Ulrich

ChangeLog:

	* infrun.c (handle_step_into_function): Do not compare a frame ID
	stack_addr directly against a read_sp () stack pointer value.
	(step_over_function): Skip signal trampoline frame on function
	return.
	* s390-tdep.c (s390_sigtramp_frame_sniffer): Move core signal
	trampoline detection logic to ...
	(s390_pc_in_sigtramp): ... this new function.
	(s390_gdbarch_init): Call set_gdbarch_pc_in_sigtramp.


Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.138
diff -c -p -r1.138 infrun.c
*** gdb/infrun.c	6 Mar 2004 00:10:06 -0000	1.138
--- gdb/infrun.c	10 Mar 2004 01:33:06 -0000
*************** handle_step_into_function (struct execut
*** 1265,1272 ****
        /* We're doing a "next".  */
  
        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
--- 1265,1271 ----
        /* We're doing a "next".  */
  
        if (pc_in_sigtramp (stop_pc)
!           && INNER_THAN (step_sp, read_sp ()))
          /* We stepped out of a signal handler, and into its
             calling trampoline.  This is misdetected as a
             subroutine call, but stepping over the signal
*************** step_over_function (struct execution_con
*** 2976,2982 ****
    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.  */
--- 2975,2982 ----
    check_for_old_step_resume_breakpoint ();
  
    if (frame_id_p (step_frame_id)
!       && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc)
!       && !pc_in_sigtramp (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.  */
Index: gdb/s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.129
diff -c -p -r1.129 s390-tdep.c
*** gdb/s390-tdep.c	26 Feb 2004 23:48:01 -0000	1.129
--- gdb/s390-tdep.c	10 Mar 2004 01:33:06 -0000
*************** static const struct frame_unwind s390_si
*** 2237,2258 ****
    s390_sigtramp_frame_prev_register
  };
  
! static const struct frame_unwind *
! s390_sigtramp_frame_sniffer (struct frame_info *next_frame)
  {
-   CORE_ADDR pc = frame_pc_unwind (next_frame);
    bfd_byte sigreturn[2];
- 
    if (read_memory_nobpt (pc, sigreturn, 2))
!     return NULL;
  
    if (sigreturn[0] != 0x0a /* svc */)
!     return NULL;
  
    if (sigreturn[1] != 119 /* sigreturn */
        && sigreturn[1] != 173 /* rt_sigreturn */)
      return NULL;
!   
    return &s390_sigtramp_frame_unwind;
  }
  
--- 2237,2266 ----
    s390_sigtramp_frame_prev_register
  };
  
! static int
! s390_pc_in_sigtramp (CORE_ADDR pc, char *name)
  {
    bfd_byte sigreturn[2];
    if (read_memory_nobpt (pc, sigreturn, 2))
!     return 0;
  
    if (sigreturn[0] != 0x0a /* svc */)
!     return 0;
  
    if (sigreturn[1] != 119 /* sigreturn */
        && sigreturn[1] != 173 /* rt_sigreturn */)
+     return 0;
+ 
+   return 1;
+ }
+ 
+ static const struct frame_unwind *
+ s390_sigtramp_frame_sniffer (struct frame_info *next_frame)
+ {
+   CORE_ADDR pc = frame_pc_unwind (next_frame);
+   if (!s390_pc_in_sigtramp (pc, NULL))
      return NULL;
! 
    return &s390_sigtramp_frame_unwind;
  }
  
*************** s390_gdbarch_init (struct gdbarch_info i
*** 3024,3029 ****
--- 3032,3038 ----
    set_gdbarch_return_value (gdbarch, s390_return_value);
  
    /* Frame handling.  */
+   set_gdbarch_pc_in_sigtramp (gdbarch, s390_pc_in_sigtramp);
    set_gdbarch_in_solib_call_trampoline (gdbarch, in_plt_section);
    dwarf2_frame_set_init_reg (gdbarch, s390_dwarf2_frame_init_reg);
    frame_unwind_append_sniffer (gdbarch, dwarf2_frame_sniffer);
-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de