Re: [offbyone RFC] Merge i386newframe

[Andrew Cagney <ac131313@redhat.com>]

[picking up old thread]

>   The need for the above suggests code trying to walk up the frame chain 
>    when it shouldn't need to.  Do you have more details?
> 
>    >  static CORE_ADDR
>    >  i386_saved_pc_after_call (struct frame_info *frame)
>    >  {
>    > -  if (get_frame_type (frame) == SIGTRAMP_FRAME)
>    > -    return i386_sigtramp_saved_pc (frame);
>    > +  char buf[4];
>    >  
>    > -  return read_memory_unsigned_integer (read_register (SP_REGNUM), 4);
>    > +  /* Our frame unwinder handles this just fine.  */
>    > +  frame_unwind_register (frame, PC_REGNUM, buf);
>    > +  return extract_address (buf, 4);
>    >  }
> 
>    Idea's for what to do with this architecture method welcome.
> 
>    I believe the intent is for this method to have relatively low overhead 
>    (when measured by the number of target interactions).  Hence, it should 
>    avoid doing prologue analysis (which frame_unwind_register() will trigger).

If that was the intent, then it no longer applies.  The call site looks 
like:

   sr_sal.pc = ADDR_BITS_REMOVE (SAVED_PC_AFTER_CALL (get_current_frame 
()));
   sr_sal.section = find_pc_overlay (sr_sal.pc);

   check_for_old_step_resume_breakpoint ();
   step_resume_breakpoint =
     set_momentary_breakpoint (sr_sal,
                               get_frame_id (get_current_frame ()),
                               bp_step_resume);

Not five lines after the SAVED_PC_AFTER_CALL call is a call to 
get_frame_id() and that is going to trigger the prologue analyser.  Kind 
of makes avoiding prologue analysis futile.

I suspect that, originally, there was a read_fp() call here (that was 
cheap) but that was later changed to get_frame_base() / get_frame_id() 
when it was realised that read_fp() was not going to be sufficient.

> Hmm.  I was under the impression that we have this function because on
> some targets (the i386 is one of them) the frame hasn't been setup yet
> when we've stopped on the first instruction of a function.

I think the prologue analyzer needs to handle this case regardless.  It 
is just an edge case of the more general problem of determing the frame 
ID when still part way through the prologue.  The d10v handles this by 
bailing out of the prologue analysis when it reaches the current 
instruction.

>    Perhaphs it should be superseeded by a method that takes a regcache 
>    instead of a frame (making the non-analysis of the prologue clearer)?
> 
> I think that would be a good idea.

On second thoughts, I'm back to thinking that deprecating it is the 
right thing to do.  Architectures need to fix their prologue analyzer.

Andrew