ppc_linux_init_extra_frame_info

ppc_linux_init_extra_frame_info

[Andreas Schwab]

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.

Andreas.

2003-02-21  Andreas Schwab  <schwab@suse.de>

	* ppc-linux-tdep.c (ppc_linux_init_extra_frame_info): Don't set
	frame type to NORMAL_FRAME now that get_prev_frame sets frame
	type before calling INIT_FRAME_INFO.

--- gdb/ppc-linux-tdep.c.~1.23.~	2003-02-04 23:12:42.000000000 +0100
+++ gdb/ppc-linux-tdep.c	2003-02-21 00:29:39.000000000 +0100
@@ -146,18 +146,7 @@ static int ppc_linux_at_sigtramp_return_
    the first instruction of the handler is stepped over instead.  That
    puts us on the second instruction.  (I added the test for the
    first instruction long after the fact, just in case the observed
-   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.)  */
 
 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);
     }
 }
 

Re: ppc_linux_init_extra_frame_info

[Andrew Cagney]

Andreas, given my comments, I think this fix is pretty funny :-)

> -   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.)  */
>  
>  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);

Re: ppc_linux_init_extra_frame_info

[Kevin Buettner]

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

Re: ppc_linux_init_extra_frame_info

[Andrew Cagney]

> 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).  */

Hmm,

ac131313@lulu$ frep deprecated_set_frame_type
frame.c:1388:deprecated_set_frame_type (struct frame_info *frame, enum 
frame_type type)
frame.h:251:extern void deprecated_set_frame_type (struct frame_info *,
i386-interix-tdep.c:166:            deprecated_set_frame_type (frame, 
SIGTRAMP_FRAME);
i386-interix-tdep.c:174:            deprecated_set_frame_type (frame, 
SIGTRAMP_FRAME);
ppc-linux-tdep.c:376:   deprecated_set_frame_type (fi, SIGTRAMP_FRAME);
ppc-linux-tdep.c:380:   deprecated_set_frame_type (fi, NORMAL_FRAME);
rs6000-tdep.c:181:    deprecated_set_frame_type (fi, SIGTRAMP_FRAME);

(Joel, I'm going to ignore interix :-/)

So it's solely PPC GNU/Linux that is the cause of all the problems.  An 
addition to the comment;

   /* FIXME: cagney/2002-11-18: Should be setting the frame's type
      here, before anything else, and not last.  Various INIT functions
      are full of work-arounds for the frames type not being set
      correctly from the word go.  Ulgh!  */
   prev->type = NORMAL_FRAME;

is that, for legacy targets, you don't know at this point what the PC is.

Why not also modify the nasty ppc-linux hack to read:

	if (get_frame_type () == NORMAL)
	  if (pc in sigtramp)
	   deprecated set frame type ()

Andrew


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

Re: ppc_linux_init_extra_frame_info

[Kevin Buettner]

On Feb 21, 11:59am, Andrew Cagney wrote:

> ac131313@lulu$ frep deprecated_set_frame_type
> frame.c:1388:deprecated_set_frame_type (struct frame_info *frame, enum 
> frame_type type)
> frame.h:251:extern void deprecated_set_frame_type (struct frame_info *,
> i386-interix-tdep.c:166:            deprecated_set_frame_type (frame, 
> SIGTRAMP_FRAME);
> i386-interix-tdep.c:174:            deprecated_set_frame_type (frame, 
> SIGTRAMP_FRAME);
> ppc-linux-tdep.c:376:   deprecated_set_frame_type (fi, SIGTRAMP_FRAME);
> ppc-linux-tdep.c:380:   deprecated_set_frame_type (fi, NORMAL_FRAME);
> rs6000-tdep.c:181:    deprecated_set_frame_type (fi, SIGTRAMP_FRAME);
> 
> (Joel, I'm going to ignore interix :-/)
> 
> So it's solely PPC GNU/Linux that is the cause of all the problems.  An 

And AIX (from rs6000-tdep.c).

> addition to the comment;
> 
>    /* FIXME: cagney/2002-11-18: Should be setting the frame's type
>       here, before anything else, and not last.  Various INIT functions
>       are full of work-arounds for the frames type not being set
>       correctly from the word go.  Ulgh!  */
>    prev->type = NORMAL_FRAME;
> 
> is that, for legacy targets, you don't know at this point what the PC is.
> 
> Why not also modify the nasty ppc-linux hack to read:
> 
> 	if (get_frame_type () == NORMAL)
> 	  if (pc in sigtramp)
> 	   deprecated set frame type ()

That's okay with me.

Kevin