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

[Andrew Cagney <cagney@gnu.org>]

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