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