Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz <drow@false.org>]

About the patch itself...

On Fri, Mar 31, 2006 at 02:17:20PM -0800, Michael Snyder wrote:
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.117
> diff -p -r1.117 target.c
> *** target.c	17 Mar 2006 00:30:34 -0000	1.117
> --- target.c	31 Mar 2006 21:46:54 -0000
> *************** update_current_target (void)
> *** 457,462 ****
> --- 457,464 ----
>         INHERIT (to_find_memory_regions, t);
>         INHERIT (to_make_corefile_notes, t);
>         INHERIT (to_get_thread_local_address, t);
> +       INHERIT (to_get_execdir, t);
> +       INHERIT (to_set_execdir, t);
>         INHERIT (to_magic, t);
>       }
>   #undef INHERIT

Need "set debug target 1" support for the new target ops.  Also, it
might be nice to use the new style rather than using INHERIT (search
the vector at call time; see the "do not inherit" comments); that makes
the debugging output nicer, too.

> +     /* Set execution direction (forward/reverse).  */
> +     int (*to_set_execdir) (enum exec_direction_kind);
> +     /* Get execution direction (forward/reverse).  */
> +     enum exec_direction_kind (*to_get_execdir) (void);
> + 

We've got these execdir methods...

> + /* Forward/reverse execution direction.  
> +    These will only be implemented by a target that supports reverse execution.
> + */

(comment formatting; wrap before the */ please?)

> + #define target_get_execution_direction() \
> +     (current_target.to_get_execdir ? \
> +      (*current_target.to_get_execdir) () : EXEC_ERROR)

... but these _execution_direction methods.  Please choose one or the
other; it makes it simpler to find the uses and implementations at the
same time.

> *************** remote_resume (ptid_t ptid, int step, en
> *** 2423,2429 ****
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (siggnal != TARGET_SIGNAL_0)
>       {
>         buf[0] = step ? 'S' : 'C';
>         buf[1] = tohex (((int) siggnal >> 4) & 0xf);
> --- 2423,2437 ----
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (target_get_execution_direction () == EXEC_REVERSE)
> !     {
> !       /* We don't pass signals to the target in reverse exec mode.  */
> !       if (info_verbose && siggnal != TARGET_SIGNAL_0)
> ! 	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
> ! 		 siggnal);
> !       strcpy (buf, step ? "bs" : "bc");
> !     }
> !   else if (siggnal != TARGET_SIGNAL_0)
>       {
>         buf[0] = step ? 'S' : 'C';
>         buf[1] = tohex (((int) siggnal >> 4) & 0xf);

You're below this bit:

  /* The vCont packet doesn't need to specify threads via Hc.  */
  if (remote_vcont_resume (ptid, step, siggnal))
    return;

So if the remote target supports vCont, you'll go forwards by accident
instead of reverse!  Talk about a traffic accident.

> + static int remote_set_execdir (enum exec_direction_kind dir)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("Set remote execdir: %s\n",
> + 		     dir == EXEC_FORWARD ? "forward" :
> + 		     dir == EXEC_REVERSE ? "reverse" :
> + 		     "bad direction");
> + 
> +   /* FIXME: check target for capability.  */

I think that's a pretty big FIXME.

> +   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
> +     return (remote_execdir = dir);

And please don't do that - no wonder Eli was confused by this
function.  If you want an assignment, put it on its own line :-)

> +   remote_ops.to_get_execdir = remote_get_execdir;
> +   remote_ops.to_set_execdir = remote_set_execdir;

What about the other remote ops vectors?  At least one isn't copied
from here.

-- 
Daniel Jacobowitz
CodeSourcery