Re: [RFA] use frame IDs to detect function calls while stepping

[Joel Brobecker <brobecker@gnat.com>]

Elena, Daniel,

> Could this kind of logic be handled inside handle_step_into_function
> (which already checks for UNDEBUGGABLE)?

Hmmm, I realize I forgot to mention that I'd like to eventually split
handle_step_into_function() into several smaller and simpler functions.
That is: Now that I have divided a large condition into smaller ones,
we might be able to write a smaller function dedicated to each case
the smaller tests handle.

This is only a wish, as I haven't had time to look more closely into
this. But I wanted to keep this option open, which is why I used the
UNDEBUGGABLE check in addition to the stop_func_name one.

> I.e. is this case caught by the complex condition in the old frame
> case which makes you call handle_step_into_function? For the new frame
> case, this is regression #1,  did you have this bug/regression with
> the old frame code?

With the old frame code (and hence using the complex condition), the
test identified by regression #1 was PASSing. The reason why it was
passing was because ecs->stop_func_name == NULL is part of the complex
condition.

This also emphasizes the fact that I slightly modified the handling of
inferior events when I added the STEP_OVER_UNDEBUGGABLE check. This
check wasn't there before (in the complex condition), and perhaps
we would all feel less nervous if I removed it, and then moved the
stop_func_name check next to the other new checks.

> > +  if (ecs->stop_func_name == NULL
> > +      && step_over_calls == STEP_OVER_UNDEBUGGABLE)
> > +    {
> > +      /* We couldn't determine where we stopped, so we just stepped
> > +         inside undebuggable code.  Since we want to step over this
> > +         kind of code, we keep going until the inferior returns from
> > +         the current function.  */
> 
> The test and the comment don't seem to match.  Code with only minimal
> Symbols will still set stop_func_name.

Perhaps the comment is a bit awkward, because I really meant that we
don't have any symbol (not even minimal) to determine where we landed.

        /* There is no symbol, not even a minimal symbol, corresponding
           to the address where we just stopped.  So we just stepped
           inside undebuggable code.  Since we want to step over this
           kind of code, we keep going until the inferior returns from
           the current function.  */

>  >   . If we are in a shlib call trampoline, then:
>  > 
>  >         Likewise.
>  >         (This test was already part of the previous check, BTW)
>  > 
>  >   . If we are in a function called from the function where we started
>  >     stepping, as identified by frame ID unwind, then:
>  > 
>  >         Likewise.
>  > 
>  > I tried it and no longer had any regression.
>  > 
> 
> I think the explanations above should go in the function as comments.

That's what I tried to do in the comments I added. Which part would
you like to see added, and where would you add it?

>  > +  else
>  > +    {
>  > +      if (IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name))
>  > +        {
>  > +          /* We landed in a shared library call trampoline, so it
>  > +             is a subroutine call.  */
>  > +          handle_step_into_function (ecs);
>  > +          return;
>  > +        }
> 
> 
> I am not sure I understand why the case above is not covered by the
> test below.  Is this to fix regression #1? I.e multiple frames? 

Hmmm, good question...

Although it does fix regression #2, it is not the only reason why we
added this test. It was also based on logic (see "After ... here is
what we found", in my previous message).

I should admit that in the case at hand (regression #2), the unwinder
is having a hard time unwinding out of this code, and causes the
frame ID check to fail. I don't remember seeing several levels of
function call.

However, I still thought that this test was necessary because we could
just as well have reached this trampoline one or more levels of function
call down, just as we end up stepping into undebuggable code in
regression #1.

-- 
Joel