Re: ppc_linux_init_extra_frame_info

[Kevin Buettner <kevinb@redhat.com>]

On Feb 21,  1:14am, Andreas Schwab wrote:

> ppc_linux_init_extra_frame_info is clobbering the frame type which
> causes dummy_frame_pop to bail out.  The frame type is now set before
> INIT_FRAME_INFO is called.

Not always, it would appear.  In create_new_frame(), the type is
set before INIT_EXTRA_FRAME_INFO() is called.  However, in
legacy_get_prev_frame(), the setting of the type occurs both
before and possibly after the call to INIT_EXTRA_FRAME_INFO().

In fact, the "after" part has me concerned.  I suspect that it could
cause a signal handler's frame to be marked as a sigtramp frame.
Andrew even comments on this:

      /* FIXME: cagney/2002-11-10: This should be moved to before the
	 INIT code above so that the INIT code knows what the frame's
	 type is (in fact, for a [generic] dummy-frame, the type can
	 be set and then the entire initialization can be skipped.
	 Unforunatly, its the INIT code that sets the PC (Hmm, catch
	 22).  */


[...]
> -   behavior is ever fixed.)
> -
> -   PC_IN_SIGTRAMP is called from blockframe.c as well in order to set
> -   the frame's type (if a SIGTRAMP_FRAME).  Because of our strange
> -   definition of in_sigtramp below, we can't rely on the frame's type
> -   getting set correctly from within blockframe.c.  This is why we
> -   take pains to set it in init_extra_frame_info().
> -
> -   NOTE: cagney/2002-11-10: I suspect the real problem here is that
> -   the get_prev_frame() only initializes the frame's type after the
> -   call to INIT_FRAME_INFO.  get_prev_frame() should be fixed, this
> -   code shouldn't be working its way around a bug :-(.  */
> +   behavior is ever fixed.)  */

I'd prefer to have these comments left in place.

I don't agree with Andrew's comment BTW.  My memory on this is rather
hazy, but I think that the problem is that we really want different
functions for PC_IN_SIGTRAMP as called from infrun.c vs frame.c.

If we could have the frame.c PC_IN_SIGRAMP calls call
ppc_linux_at_sigtramp_return_path() and the infrun.c calls call
ppc_linux_in_sigtramp(), things would work right.  This suggests
that we really need two different methods.

(Take my current remarks with a grain of salt.  It's been a while
since I've worked on this code...)

If you'd like to add you own notes to this comment block (or anywhere
else), I have no problem with that.

>  int
>  ppc_linux_in_sigtramp (CORE_ADDR pc, char *func_name)
> @@ -374,10 +363,6 @@ ppc_linux_init_extra_frame_info (int fro
>           at trampoline code */
>        if (ppc_linux_at_sigtramp_return_path (fi->pc))
>  	deprecated_set_frame_type (fi, SIGTRAMP_FRAME);
> -      else
> -	/* FIXME: cagney/2002-11-10: Is this double bogus?  What
> -           happens if the frame has previously been marked as a dummy?  */
> -	deprecated_set_frame_type (fi, NORMAL_FRAME);
>      }
>  }

This part is fine.  (I.e, I think this part should be committed.)

Thanks,

Kevin