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

Andrew Cagney
2004-03-11T16:08:36Z

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