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

Andrew Cagney
2004-03-15T17:12:15Z

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