Re: [RFA] OSF/1 - "next" over prologueless function call

[Andrew Cagney <cagney@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 5495 bytes --]

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


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 13167 bytes --]

1.155        (grossman 24-Oct-95): #if 0
1.155        (grossman 24-Oct-95):       /* I disabled this test because it was too complicated and slow.  The
1.155        (grossman 24-Oct-95): 	 SKIP_PROLOGUE was especially slow, because it caused unnecessary
1.155        (grossman 24-Oct-95): 	 prologue examination on various architectures.  The code in the #else
1.155        (grossman 24-Oct-95): 	 clause has been tested on the Sparc, Mips, PA, and Power
1.155        (grossman 24-Oct-95): 	 architectures, so it's pretty likely to be correct.  -Stu 10/24/95 */
1.155        (grossman 24-Oct-95): 
1.137        (kingdon  08-Oct-94):       /* See if we left the step range due to a subroutine call that
1.137        (kingdon  08-Oct-94): 	 we should proceed to the end of.  */
1.137        (kingdon  08-Oct-94): 
1.80         (kingdon  11-Jul-93):       if (stop_func_start)
1.80         (kingdon  11-Jul-93): 	{
1.130        (grossman 02-Jun-94): 	  struct symtab *s;
1.130        (grossman 02-Jun-94): 
1.80         (kingdon  11-Jul-93): 	  /* Do this after the IN_SIGTRAMP check; it might give
1.80         (kingdon  11-Jul-93): 	     an error.  */
1.80         (kingdon  11-Jul-93): 	  prologue_pc = stop_func_start;
1.130        (grossman 02-Jun-94): 
1.130        (grossman 02-Jun-94): 	  /* Don't skip the prologue if this is assembly source */
1.130        (grossman 02-Jun-94): 	  s = find_pc_symtab (stop_pc);
1.130        (grossman 02-Jun-94): 	  if (s && s->language != language_asm)
1.130        (grossman 02-Jun-94): 	    SKIP_PROLOGUE (prologue_pc);
1.80         (kingdon  11-Jul-93): 	}
1.1          (rich     28-Mar-91): 
1.101        (kingdon  17-Oct-93):       if ((/* Might be a non-recursive call.  If the symbols are missing
1.101        (kingdon  17-Oct-93): 	      enough that stop_func_start == prev_func_start even though
1.101        (kingdon  17-Oct-93): 	      they are really two functions, we will treat some calls as
1.101        (kingdon  17-Oct-93): 	      jumps.  */
1.101        (kingdon  17-Oct-93): 	   stop_func_start != prev_func_start
1.101        (kingdon  17-Oct-93): 
1.101        (kingdon  17-Oct-93): 	   /* Might be a recursive call if either we have a prologue
1.101        (kingdon  17-Oct-93): 	      or the call instruction itself saves the PC on the stack.  */
1.101        (kingdon  17-Oct-93): 	   || prologue_pc != stop_func_start
1.137        (kingdon  08-Oct-94): 	   || read_sp () != step_sp)
1.105        (grossman 26-Oct-93): 	  && (/* PC is completely out of bounds of any known objfiles.  Treat
1.105        (grossman 26-Oct-93): 		 like a subroutine call. */
1.105        (grossman 26-Oct-93): 	      ! stop_func_start
1.101        (kingdon  17-Oct-93): 
1.110        (kingdon  30-Dec-93): 	      /* If we do a call, we will be at the start of a function...  */
1.101        (kingdon  17-Oct-93): 	      || stop_pc == stop_func_start
1.37         (bothner  01-Mar-92): 
1.110        (kingdon  30-Dec-93): 	      /* ...except on the Alpha with -O (and also Irix 5 and
1.110        (kingdon  30-Dec-93): 		 perhaps others), in which we might call the address
1.110        (kingdon  30-Dec-93): 		 after the load of gp.  Since prologues don't contain
1.110        (kingdon  30-Dec-93): 		 calls, we can't return to within one, and we don't
1.110        (kingdon  30-Dec-93): 		 jump back into them, so this check is OK.  */
1.110        (kingdon  30-Dec-93): 
1.101        (kingdon  17-Oct-93): 	      || stop_pc < prologue_pc
1.101        (kingdon  17-Oct-93): 
1.134        (schauer  15-Jul-94): 	      /* ...and if it is a leaf function, the prologue might
1.134        (schauer  15-Jul-94):  		 consist of gp loading only, so the call transfers to
1.134        (schauer  15-Jul-94):  		 the first instruction after the prologue.  */
1.134        (schauer  15-Jul-94):  	      || (stop_pc == prologue_pc
1.134        (schauer  15-Jul-94): 
1.134        (schauer  15-Jul-94): 		  /* Distinguish this from the case where we jump back
1.134        (schauer  15-Jul-94): 		     to the first instruction after the prologue,
1.134        (schauer  15-Jul-94): 		     within a function.  */
1.134        (schauer  15-Jul-94): 		   && stop_func_start != prev_func_start)
1.134        (schauer  15-Jul-94): 
1.101        (kingdon  17-Oct-93): 	      /* If we end up in certain places, it means we did a subroutine
1.101        (kingdon  17-Oct-93): 		 call.  I'm not completely sure this is necessary now that we
1.101        (kingdon  17-Oct-93): 		 have the above checks with stop_func_start (and now that
1.102        (kingdon  17-Oct-93): 		 find_pc_partial_function is pickier).  */
1.140        (law      24-Nov-94): 	      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, stop_func_name)
1.101        (kingdon  17-Oct-93): 
1.101        (kingdon  17-Oct-93): 	      /* If none of the above apply, it is a jump within a function,
1.101        (kingdon  17-Oct-93): 		 or a return from a subroutine.  The other case is longjmp,
1.101        (kingdon  17-Oct-93): 		 which can no longer happen here as long as the
1.101        (kingdon  17-Oct-93): 		 handling_longjmp stuff is working.  */
1.101        (kingdon  17-Oct-93): 	      ))
1.130        (grossman 02-Jun-94): #else
1.130        (grossman 02-Jun-94): /* This is experimental code which greatly simplifies the subroutine call
1.130        (grossman 02-Jun-94):    test.  I've actually tested on the Alpha, and it works great. -Stu */
1.130        (grossman 02-Jun-94): 
1.155        (grossman 24-Oct-95): 	if (stop_pc == stop_func_start /* Quick test */
1.155        (grossman 24-Oct-95): 	    || in_prologue (stop_pc, stop_func_start)
1.155        (grossman 24-Oct-95): 	    || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, stop_func_name)
1.155        (grossman 24-Oct-95): 	    || stop_func_start == 0)
1.130        (grossman 02-Jun-94): #endif
1.155        (grossman 24-Oct-95): 
1.80         (kingdon  11-Jul-93): 	{
1.80         (kingdon  11-Jul-93): 	  /* It's a subroutine call.  */
1.34         (grossman 22-Feb-92): 
1.80         (kingdon  11-Jul-93): 	  if (step_over_calls == 0)
1.80         (kingdon  11-Jul-93): 	    {
1.80         (kingdon  11-Jul-93): 	      /* I presume that step_over_calls is only 0 when we're
1.80         (kingdon  11-Jul-93): 		 supposed to be stepping at the assembly language level
1.80         (kingdon  11-Jul-93): 		 ("stepi").  Just stop.  */
1.80         (kingdon  11-Jul-93): 	      stop_step = 1;
1.80         (kingdon  11-Jul-93): 	      break;
1.80         (kingdon  11-Jul-93): 	    }
1.37         (bothner  01-Mar-92): 
1.80         (kingdon  11-Jul-93): 	  if (step_over_calls > 0)
1.80         (kingdon  11-Jul-93): 	    /* We're doing a "next".  */
1.80         (kingdon  11-Jul-93): 	    goto step_over_function;
1.80         (kingdon  11-Jul-93): 
1.80         (kingdon  11-Jul-93): 	  /* If we are in a function call trampoline (a stub between
1.80         (kingdon  11-Jul-93): 	     the calling routine and the real function), locate the real
1.80         (kingdon  11-Jul-93): 	     function.  That's what tells us (a) whether we want to step
1.80         (kingdon  11-Jul-93): 	     into it at all, and (b) what prologue we want to run to
1.80         (kingdon  11-Jul-93): 	     the end of, if we do step into it.  */
1.80         (kingdon  11-Jul-93): 	  tmp = SKIP_TRAMPOLINE_CODE (stop_pc);
1.80         (kingdon  11-Jul-93): 	  if (tmp != 0)
1.80         (kingdon  11-Jul-93): 	    stop_func_start = tmp;
1.80         (kingdon  11-Jul-93): 
1.80         (kingdon  11-Jul-93): 	  /* If we have line number information for the function we
1.80         (kingdon  11-Jul-93): 	     are thinking of stepping into, step into it.
1.80         (kingdon  11-Jul-93): 
1.80         (kingdon  11-Jul-93): 	     If there are several symtabs at that PC (e.g. with include
1.80         (kingdon  11-Jul-93): 	     files), just want to know whether *any* of them have line
1.80         (kingdon  11-Jul-93): 	     numbers.  find_pc_line handles this.  */
1.80         (kingdon  11-Jul-93): 	  {
1.80         (kingdon  11-Jul-93): 	    struct symtab_and_line tmp_sal;
1.77         (kingdon  29-Jun-93): 
1.80         (kingdon  11-Jul-93): 	    tmp_sal = find_pc_line (stop_func_start, 0);
1.80         (kingdon  11-Jul-93): 	    if (tmp_sal.line != 0)
1.80         (kingdon  11-Jul-93): 	      goto step_into_function;
1.80         (kingdon  11-Jul-93): 	  }
1.77         (kingdon  29-Jun-93): 
1.37         (bothner  01-Mar-92): step_over_function:
1.80         (kingdon  11-Jul-93): 	  /* A subroutine call has happened.  */
1.80         (kingdon  11-Jul-93): 	  {
1.80         (kingdon  11-Jul-93): 	    /* Set a special breakpoint after the return */
1.80         (kingdon  11-Jul-93): 	    struct symtab_and_line sr_sal;
1.80         (kingdon  11-Jul-93): 	    sr_sal.pc = 
1.80         (kingdon  11-Jul-93): 	      ADDR_BITS_REMOVE
1.80         (kingdon  11-Jul-93): 		(SAVED_PC_AFTER_CALL (get_current_frame ()));
1.80         (kingdon  11-Jul-93): 	    sr_sal.symtab = NULL;
1.80         (kingdon  11-Jul-93): 	    sr_sal.line = 0;
1.80         (kingdon  11-Jul-93): 	    step_resume_breakpoint =
1.80         (kingdon  11-Jul-93): 	      set_momentary_breakpoint (sr_sal, get_current_frame (),
1.80         (kingdon  11-Jul-93): 					bp_step_resume);
1.137        (kingdon  08-Oct-94): 	    step_resume_breakpoint->frame = step_frame_address;
1.80         (kingdon  11-Jul-93): 	    if (breakpoints_inserted)
1.80         (kingdon  11-Jul-93): 	      insert_breakpoints ();
1.80         (kingdon  11-Jul-93): 	  }
1.80         (kingdon  11-Jul-93): 	  goto keep_going;
1.34         (grossman 22-Feb-92): 
1.37         (bothner  01-Mar-92): step_into_function:
1.80         (kingdon  11-Jul-93): 	  /* Subroutine call with source code we should not step over.
1.80         (kingdon  11-Jul-93): 	     Do step to the first line of code in it.  */
1.130        (grossman 02-Jun-94): 	  {
1.130        (grossman 02-Jun-94): 	    struct symtab *s;
1.130        (grossman 02-Jun-94): 
1.130        (grossman 02-Jun-94): 	    s = find_pc_symtab (stop_pc);
1.130        (grossman 02-Jun-94): 	    if (s && s->language != language_asm)
1.130        (grossman 02-Jun-94): 	      SKIP_PROLOGUE (stop_func_start);
1.130        (grossman 02-Jun-94): 	  }
1.80         (kingdon  11-Jul-93): 	  sal = find_pc_line (stop_func_start, 0);
1.80         (kingdon  11-Jul-93): 	  /* Use the step_resume_break to step until
1.80         (kingdon  11-Jul-93): 	     the end of the prologue, even if that involves jumps
1.80         (kingdon  11-Jul-93): 	     (as it seems to on the vax under 4.2).  */
1.80         (kingdon  11-Jul-93): 	  /* If the prologue ends in the middle of a source line,
1.111        (schauer  01-Jan-94): 	     continue to the end of that source line (if it is still
1.111        (schauer  01-Jan-94): 	     within the function).  Otherwise, just go to end of prologue.  */
1.1          (rich     28-Mar-91): #ifdef PROLOGUE_FIRSTLINE_OVERLAP
1.80         (kingdon  11-Jul-93): 	  /* no, don't either.  It skips any code that's
1.80         (kingdon  11-Jul-93): 	     legitimately on the first line.  */
1.1          (rich     28-Mar-91): #else
1.111        (schauer  01-Jan-94): 	  if (sal.end && sal.pc != stop_func_start && sal.end < stop_func_end)
1.80         (kingdon  11-Jul-93): 	    stop_func_start = sal.end;
1.1          (rich     28-Mar-91): #endif
1.37         (bothner  01-Mar-92): 
1.80         (kingdon  11-Jul-93): 	  if (stop_func_start == stop_pc)
1.80         (kingdon  11-Jul-93): 	    {
1.80         (kingdon  11-Jul-93): 	      /* We are already there: stop now.  */
1.80         (kingdon  11-Jul-93): 	      stop_step = 1;
1.80         (kingdon  11-Jul-93): 	      break;
1.80         (kingdon  11-Jul-93): 	    }
1.80         (kingdon  11-Jul-93): 	  else
1.80         (kingdon  11-Jul-93): 	    /* Put the step-breakpoint there and go until there. */
1.80         (kingdon  11-Jul-93): 	    {
1.80         (kingdon  11-Jul-93): 	      struct symtab_and_line sr_sal;
1.80         (kingdon  11-Jul-93): 
1.80         (kingdon  11-Jul-93): 	      sr_sal.pc = stop_func_start;
1.80         (kingdon  11-Jul-93): 	      sr_sal.symtab = NULL;
1.80         (kingdon  11-Jul-93): 	      sr_sal.line = 0;
1.80         (kingdon  11-Jul-93): 	      /* Do not specify what the fp should be when we stop
1.80         (kingdon  11-Jul-93): 		 since on some machines the prologue
1.80         (kingdon  11-Jul-93): 		 is where the new fp value is established.  */
1.80         (kingdon  11-Jul-93): 	      step_resume_breakpoint =
1.89         (kingdon  18-Sep-93): 		set_momentary_breakpoint (sr_sal, NULL, bp_step_resume);
1.80         (kingdon  11-Jul-93): 	      if (breakpoints_inserted)
1.80         (kingdon  11-Jul-93): 		insert_breakpoints ();
1.80         (kingdon  11-Jul-93): 
1.80         (kingdon  11-Jul-93): 	      /* And make sure stepping stops right away then.  */
1.80         (kingdon  11-Jul-93): 	      step_range_end = step_range_start;
1.1          (rich     28-Mar-91): 	    }
1.80         (kingdon  11-Jul-93): 	  goto keep_going;
1.80         (kingdon  11-Jul-93): 	}