Re: ppc_linux_init_extra_frame_info

[Andrew Cagney <ac131313@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).  */

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
>