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

[Joel Brobecker <brobecker at gnat dot com>]

> (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 :-).

> >However, I tried it on sparc-solaris with the gdb-6.0 sources because
> >I knew this target hasn't transitioned to the new frame framework.
> >It doesn't look like GDB is liking this change there (I've got a lot of
> >timeouts in call-ar-st). I am currently retrying on the head right now,
> >hoping the testsuite completes in a reasonable amount of time.

I redid the testing this time using the head, and found no new
regression introduced by this change [1].

> >--- infrun.c    25 Nov 2003 16:01:36 -0000      1.122
> >+++ infrun.c    3 Dec 2003 19:24:07 -0000
> >@@ -2473,6 +2473,8 @@ process_event_stop_test:
> >     }
> >
> >   if (((stop_pc == ecs->stop_func_start        /* Quick test */
> >+        || 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)


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

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.

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.

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

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.

Thanks,
-- 
Joel

[1]: There was one glitch that occurred during the testing, though.
     Even before I applied any change, the structs.exp test keeps
     timeouting. I tried letting it complete, but had to stop it
     after an entire night of timeouts. So I did my testing after
     having deactivated this test.