Re: [RFA] Resubmit process record and replay, 7/10

[Pedro Alves <pedro@codesourcery.com>]

My 2c.,

On Sunday 16 November 2008 08:23:12, teawater wrote:
> --- a/linux-nat.c
> +++ b/linux-nat.c
> @@ -50,6 +50,8 @@
>  #include "event-loop.h"
>  #include "event-top.h"
>  
> +#include "record.h"
> +
>  #ifdef HAVE_PERSONALITY
>  # include <sys/personality.h>
>  # if !HAVE_DECL_ADDR_NO_RANDOMIZE
> @@ -518,6 +520,115 @@ my_waitpid (int pid, int *status, int fl
>    return ret;
>  }
>  

Can you please try moving all this blob...

> +extern struct bp_location *bp_location_chain;
> +static struct lwp_info * find_lwp_pid (ptid_t ptid);
> +static int
> +my_waitpid_record (int pid, int *status, int flags)
> +{
> +  int ret;
> +  struct bp_location *bl;
> +  struct breakpoint *b;
> +  CORE_ADDR pc;
> +  CORE_ADDR decr_pc_after_break;
> +  struct lwp_info *lp;
> +  int is_breakpoint = 1;
> +
> +wait_begin:
> +  ret = my_waitpid (pid, status, flags);
> +  if (ret == -1)
> +    {
> +      return ret;
> +    }
> +
> +  if (ret == 0)
> +    {
> +      goto wait_begin;
> +    }
> +
> +  if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
> +    {
> +      /* Check if there is a breakpoint.  */
> +      pc = 0;
> +      registers_changed ();
> +      for (bl = bp_location_chain; bl; bl = bl->global_next)
> +       {
> +         b = bl->owner;
> +         gdb_assert (b);
> +         if (b->enable_state != bp_enabled
> +             && b->enable_state != bp_permanent)
> +           continue;
> +         if (!pc)
> +           {
> +             pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
> +           }
> +         switch (b->type)
> +           {
> +           default:
> +             if (bl->address == pc)
> +               {
> +                 goto out;
> +               }
> +             break;
> +
> +           case bp_watchpoint:
> +             /*XXX teawater: I still not very clear how to deal with it.  */
> +             goto out;
> +             break;
> +
> +           case bp_catchpoint:
> +             gdb_assert (b->ops != NULL && b->ops->breakpoint_hit != NULL);
> +             if (b->ops->breakpoint_hit (b))
> +               {
> +                 goto out;
> +               }
> +             break;
> +
> +           case bp_hardware_watchpoint:
> +           case bp_read_watchpoint:
> +           case bp_access_watchpoint:
> +             if (STOPPED_BY_WATCHPOINT (0))
> +               {
> +                 goto out;
> +               }
> +             break;
> +           }
> +       }
> +
> +      lp = find_lwp_pid (pid_to_ptid (ret));
> +      if (lp)
> +        lp->stopped = 1;
> +
> +      /* record message */
> +      record_message (current_gdbarch);
> +
> +      /* resume program */
> +      linux_ops->to_resume (pid_to_ptid (ret), 1, TARGET_SIGNAL_0);
> +      goto wait_begin;
> +    }
> +
> +  is_breakpoint = 0;
> +
> +out:
> +  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address
> +     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
> +  if (is_breakpoint)
> +    {
> +      decr_pc_after_break = gdbarch_decr_pc_after_break
> +       (get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
> +      if (decr_pc_after_break)
> +       {
> +         if (!pc)
> +           {
> +             pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
> +           }
> +         regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
> +                            pc + decr_pc_after_break);
> +       }
> +    }
> +
> +  return ret;
> +}
> +

... to the record target?  It seems to be interested in getting
*all* events, instead of letting linux_nat_wait filter some.

>  /* Determine if PTRACE_O_TRACEFORK can be used to follow fork events.
>  
>     First, we try to enable fork tracing on ORIGINAL_PID.  If this fails,
> @@ -2876,7 +2987,16 @@ retry:
>            queued events.  */
>         lwpid = queued_waitpid (pid, &status, options);
>        else
> -       lwpid = my_waitpid (pid, &status, options);
> +       {
> +         if (RECORD_IS_USED && !record_resume_step)
> +           {
> +             lwpid = my_waitpid_record (pid, &status, options);
> +           }
> +         else
> +           {
> +             lwpid = my_waitpid (pid, &status, options);
> +           }
> +       }
>  

Could you do it by instead of calling my_waitpid_record here, make
sure that whatever comes out of my_waitpid results in returning from
linux_nat_wait?

Ideally, we should have a flags parameter in target_wait, like:

 -ptid_t target_wait (ptid_t, struct target_waitstatus *);
 +ptid_t target_wait (ptid_t, struct target_waitstatus *, int target_flags);

... process record would pass a special flag to
linux_nat_wait for this, but I'd be happy if you tried moving
most of the code to the record target, and kept using RECORD_IS_USED,
as I hinted at in another message a new target_is_recording_p () or
somesuch method.

Also, it seems most of the breakpoint checking code should be replaced
by one of the breakpoint_here style predicates exported by breakpoint.h.

-- 
Pedro Alves