[RFA] Handle runtime loader dyn sym resolution in reverse

[RFA] Handle runtime loader dyn sym resolution in reverse

[Michael Snyder]

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

This is for both the main trunk and the record/replay branch
(so I'd appreciate review by both Hui and a global maintainer).

It fixes a bug where you try to "reverse-next" over a
first-time dynamic function call (eg. printf).  Infrun
would get lost when it stepped into the jump table
backwards.


[-- Attachment #2: shmain.txt --]
[-- Type: text/plain, Size: 1210 bytes --]

2008-10-24  Michael Snyder  <msnyder@vmware.com>

	* infrun.c (handle_inferior_event): Handle dynamic symbol
	resolution in reverse.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.322.2.14
diff -u -p -r1.322.2.14 infrun.c
--- infrun.c	23 Oct 2008 23:24:45 -0000	1.322.2.14
+++ infrun.c	25 Oct 2008 02:57:37 -0000
@@ -3212,6 +3212,22 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 	  if (execution_direction == EXEC_REVERSE)
 	    {
 	      struct symtab_and_line sr_sal;
+
+	      if (ecs->stop_func_start == 0 
+		  && in_solib_dynsym_resolve_code (stop_pc))
+		{
+		  /* Stepped into runtime loader dynamic symbol
+		     resolution code.  Since we're in reverse, 
+		     we have already backed up through the runtime
+		     loader and the dynamic function.  This is just
+		     the trampoline (jump table).
+
+		     Just keep stepping, we'll soon be home.
+		  */
+		  keep_going (ecs);
+		  return;
+		}
+	      /* Normal (staticly linked) function call return.  */
 	      init_sal (&sr_sal);
 	      sr_sal.pc = ecs->stop_func_start;
 	      insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);

Re: [RFA] Handle runtime loader dyn sym resolution in reverse

[teawater]

Great! It's OK to me. Please check it into 0930 branch and main tree.

On Sat, Oct 25, 2008 at 10:58, Michael Snyder <msnyder@vmware.com> wrote:
> This is for both the main trunk and the record/replay branch
> (so I'd appreciate review by both Hui and a global maintainer).
>
> It fixes a bug where you try to "reverse-next" over a
> first-time dynamic function call (eg. printf).  Infrun
> would get lost when it stepped into the jump table
> backwards.
>
>
> 2008-10-24  Michael Snyder  <msnyder@vmware.com>
>
>        * infrun.c (handle_inferior_event): Handle dynamic symbol
>        resolution in reverse.
>
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.322.2.14
> diff -u -p -r1.322.2.14 infrun.c
> --- infrun.c    23 Oct 2008 23:24:45 -0000      1.322.2.14
> +++ infrun.c    25 Oct 2008 02:57:37 -0000
> @@ -3212,6 +3212,22 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>          if (execution_direction == EXEC_REVERSE)
>            {
>              struct symtab_and_line sr_sal;
> +
> +             if (ecs->stop_func_start == 0
> +                 && in_solib_dynsym_resolve_code (stop_pc))
> +               {
> +                 /* Stepped into runtime loader dynamic symbol
> +                    resolution code.  Since we're in reverse,
> +                    we have already backed up through the runtime
> +                    loader and the dynamic function.  This is just
> +                    the trampoline (jump table).
> +
> +                    Just keep stepping, we'll soon be home.
> +                 */
> +                 keep_going (ecs);
> +                 return;
> +               }
> +             /* Normal (staticly linked) function call return.  */
>              init_sal (&sr_sal);
>              sr_sal.pc = ecs->stop_func_start;
>              insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
>
>

Re: [RFA] Handle runtime loader dyn sym resolution in reverse

[Joel Brobecker]

> This is for both the main trunk and the record/replay branch
> (so I'd appreciate review by both Hui and a global maintainer).

I took a look. The patch itself is missing a little bit of context
(in terms of the code), so I wasn't sure whether I was looking at
inserting this hunk at the right place, but I think I found it:

      if (ecs->event_thread->step_over_calls == STEP_OVER_ALL)
        {
          /* We're doing a "next".

             Normal (forward) execution: set a breakpoint at the
             callee's return address (the address at which the caller
             will resume).

             Reverse (backward) execution.  set the step-resume
             breakpoint at the start of the function that we just
             stepped into (backwards), and continue to there.  When we
             get there, we'll need to single-step back to the caller.  */

          if (execution_direction == EXEC_REVERSE)

> 2008-10-24  Michael Snyder  <msnyder@vmware.com>
> 
> 	* infrun.c (handle_inferior_event): Handle dynamic symbol
> 	resolution in reverse.

Looks OK to me, but I don't understand why you check that stop_func_start
is zero. If I were to make a guess, it would be to quickly dismiss
the case were we reverse stepped into a function call, and so don't
need to check for trampolines (in other words, a CPU saver).

-- 
Joel

Re: [RFA] Handle runtime loader dyn sym resolution in reverse

[teawater]

Hi Joel,

This patch fix the bug that I send in

http://sourceware.org/ml/gdb-patches/2008-10/msg00485.html

Thanks,
Hui

On Sun, Oct 26, 2008 at 00:09, Joel Brobecker <brobecker@adacore.com> wrote:
>> This is for both the main trunk and the record/replay branch
>> (so I'd appreciate review by both Hui and a global maintainer).
>
> I took a look. The patch itself is missing a little bit of context
> (in terms of the code), so I wasn't sure whether I was looking at
> inserting this hunk at the right place, but I think I found it:
>
>      if (ecs->event_thread->step_over_calls == STEP_OVER_ALL)
>        {
>          /* We're doing a "next".
>
>             Normal (forward) execution: set a breakpoint at the
>             callee's return address (the address at which the caller
>             will resume).
>
>             Reverse (backward) execution.  set the step-resume
>             breakpoint at the start of the function that we just
>             stepped into (backwards), and continue to there.  When we
>             get there, we'll need to single-step back to the caller.  */
>
>          if (execution_direction == EXEC_REVERSE)
>
>> 2008-10-24  Michael Snyder  <msnyder@vmware.com>
>>
>>       * infrun.c (handle_inferior_event): Handle dynamic symbol
>>       resolution in reverse.
>
> Looks OK to me, but I don't understand why you check that stop_func_start
> is zero. If I were to make a guess, it would be to quickly dismiss
> the case were we reverse stepped into a function call, and so don't
> need to check for trampolines (in other words, a CPU saver).
>
> --
> Joel
>

Re: [RFA] Handle runtime loader dyn sym resolution in reverse

[Michael Snyder]

Joel Brobecker wrote:

> 
>> 2008-10-24  Michael Snyder  <msnyder@vmware.com>
>>
>>       * infrun.c (handle_inferior_event): Handle dynamic symbol
>>       resolution in reverse.
> 
> Looks OK to me, but I don't understand why you check that stop_func_start
> is zero. If I were to make a guess, it would be to quickly dismiss
> the case were we reverse stepped into a function call, and so don't
> need to check for trampolines (in other words, a CPU saver).

Correct, but it's more than a CPU saver.
If we are in a function (stop_func_start is non-zero),
then we can set a step_resume breakpoint and continue,
which is way faster than single stepping.

We single-step only when we have to, ie. when we have
actually stepped into the fixup jump table.

Re: [RFA] Handle runtime loader dyn sym resolution in reverse

[Michael Snyder]

Michael Snyder wrote:
> Joel Brobecker wrote:
> 
>>> 2008-10-24  Michael Snyder  <msnyder@vmware.com>
>>>
>>>       * infrun.c (handle_inferior_event): Handle dynamic symbol
>>>       resolution in reverse.
>> Looks OK to me, but I don't understand why you check that stop_func_start
>> is zero. If I were to make a guess, it would be to quickly dismiss
>> the case were we reverse stepped into a function call, and so don't
>> need to check for trampolines (in other words, a CPU saver).
> 
> Correct, but it's more than a CPU saver.
> If we are in a function (stop_func_start is non-zero),
> then we can set a step_resume breakpoint and continue,
> which is way faster than single stepping.
> 
> We single-step only when we have to, ie. when we have
> actually stepped into the fixup jump table.

The big difference is, 'in_solib_dynsym_resolve_code'
returns true when we're in the dynamic loader function
"_dl_runtime_resolve", but we really don't want to single
step through that.  Since it's a function with a minsym,
we can jump over it just like any other function -- but
then we hit the jump table and have to singlestep a few
times.

The test case was sent to me by teawater, who tried to
reverse-next over a printf.  GDB handled the printf call
fine, then it stepped into _dl_runtime_resolve, which it
also handled, but it stumbled over the jump table.

Committed, by the way.