> (only took 15 years to realise how "obvious" it was :-)
> 
> 
> Right. It's only "obvious" because the we now have a frame ID which
> particularity is to be unique... Given how long it must have taken to
> "invent the wheel", maybe we didn't do too bad :-).

See the comments in the attached ...

>> You can use legacy_frame_p for differentiating old and new code.
> 
> 
> Yes. Despite the relative success on sparc-solaris (no new regression),
> I wasn't sure whether this wasn't potentially introducing new problems
> on the architectures where the legacy frames are still used. So I added
> it to the condition.
> 
> That gives us a pretty horrible condition, something like that:
> 
>    if (((stop_pc == ecs->stop_func_start        /* Quick test */
>          || (!legacy_frame_pd (current_gdbarch)
>              && frame_id_eq
>                   (get_frame_id (get_prev_frame (get_current_frame ())),
>                    step_frame_id))
>          || in_prologue (stop_pc, ecs->stop_func_start))
>         && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
>        || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
>        || ecs->stop_func_name == 0)
> 
> How about we move this into a function?

There are several things we could move to a function ....

> My current version only replaces the condition I added by:
> 
>        || frame_in_procedure_called_during_step ()
> 
> So the condition stay relatively readable (not too many levels in terms
> of nesting). But maybe it's time to turn the entire condition into a
> function and add more comments.
> 
> Something like this?
> 
> static int
> inside_function_called_from_step (CORE_ADDR stop_pc,
>                                   CORE_ADDR stop_func_start)
> {
>   /* If bla bla bla  */
>   if (stop_pc == stop_func_start
>       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name))
>     return 1;
> 
>   /* If the frame ID of the previous frame is equal to step_frame_id,
>      then this function was indeed called during a step.  Only rely
>      on this test if we don't use the legacy frames.  */
>   if (!legacy_frame_p (current_gdbarch)
>       && frame_id_eq (...)
>       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name))
>     return 1;
> 
>   /* cagney/2003-12-04: Do we really need this, now that we check the
>      previous frame id?  */
>   if (in_prologue (stop_pc, ecs->stop_func_start))
>       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, stop_func_name))
>     return 1;
> 
>   /* Don't stop stepping inside a solib call trampoline.  */
>   if (IN_SOLIB_CALL_TRAMPOLINE (stop_pc, stop_func_name))
>     return 1;
> 
>   /* Hmmm, continue stepping if we don't know anything about where
>      we landed.  */
>   if (stop_func_name == 0)
>      return 1;
>   
>   return 0;
> }
> 
> The trouble is that this change is not completly mechanical. And
> we also end up repeating the "!IN_SOLIB_CALL_TRAMPOLINE" test.
> We can also coalesce the first 3 tests into one if, but then
> we lose a bit the advantage of using a function.

There's always plan B.

Looking at the body of that IF, I believe it always returns.  That 
should let us do:

if (legacy_frame_p ())
   if (all the existing tests)
     call a function to do the body of work ()
     return;
else if (our new improved test)
   call a function to do the body of work ()
   return;

that way the legacy and non legacy cases are clearly split - we're free 
to refine the new conditional with out worrying about breaking the old 
code.  However ....

(Oh, what exactly is that body of code doing - any ideas?)

> Anyway, it's more a matter of style, so I will defer to the preference
> of the maintainers, and send a patch along these lines. If we decide
> to move the entire condition into a new function, I can send 2 patches:
> One that moves the current condition, and then another that fixes
> the osf1 problem.
> 
> 
>> Hmm, is "stop_pc == ecs->stop_func_start" a valid test, what happens if 
>> the program is at 1: and there's a next?
>> 
>> 	foo:
>> 		...
>> 	1:	goto foo
> 
> 
> Unless the function is completely prologueless, all should be well
> because the goto will branch at a location past the first instruction.
> If the function is prologueless, however... I think this would be a rare
> occurrence.

I'm not so sure :-(  In fact any lack of such code suggests a GCC 
optimizer bug :-)

>> Hmm, is in_prologue() adding value when frame_id always works?  Unless 
>> it's being used to handle stepping through a prologue?
> 
> 
> Not sure about the real intent of in_prologue(). I know it saved us
> most of the time in the osf/1 case, but was it the primary intent
> when this condition was added? I think that once we have answered
> this question, we will be able to decide whether or not this condition
> can be removed.

...  valid point.

Staring at that code its doing so many things its depressing:

- step into function
- step over function
- step out of function
- step over trampoline

but the basic intent was possibly step into a function and then skip 
over its prologue?

So I'm guessing for the moment just replace
	stop_pc == ecs->stop_func_start
with the frame id test in the new code?

:-(

> I also realize that I may not have been clear about the usage of
> legacy_frame_p. I am a bit nervous at doing without, but you may be
> more confident than me, since I don't have much knowledge of the new
> frame architecture yet. So the choice of using it or not is still open.

Andrew