Re: [RFA/RFC] Add dump and load command to process record and replay

[Michael Snyder <msnyder at vmware dot com>]

I think it's too easy to get the debugger into an internally
inconsistent state.  If you don't know *exactly* what you are
doing, this is too likely to go wrong.

Sometimes we have to choose between flexibility and letting
the user shoot himself in the foot.

Hui Zhu wrote:
> About it, I think keep the old one can make the function more
> flexible. I try it sometime.  It can make two or more gdb_record files
> to one file.
>
> What about add a warning to there? When the record list is not empty,
> output a warning.
>
> Thanks,
> Hui
>
> On Fri, Aug 7, 2009 at 11:04, Michael Snyder<msnyder@vmware.com> wrote:
> > Hui Zhu wrote:
> >
> > > +/* Load the execution log from a file.  */
> > > +
> > > +static void
> > > +cmd_record_load (char *args, int from_tty)
> > > +{
> > > +  int recfd;
> > > +  uint32_t magic;
> > > +  struct cleanup *old_cleanups;
> > > +  struct cleanup *old_cleanups2;
> > > +  struct record_entry *rec;
> > > +  int insn_number = 0;
> > > +
> > > +  if (current_target.to_stratum != record_stratum)
> > > +    {
> > > +      cmd_record_start (NULL, from_tty);
> > > +      printf_unfiltered (_("Auto start process record.\n"));
> > > +    }
> > > +
> > > +  if (!args || (args && !*args))
> > > +    error (_("Argument for filename required.\n"));
> > > +
> > > +  /* Open the load file.  */
> > > +  recfd = open (args, O_RDONLY | O_BINARY);
> > > +  if (recfd < 0)
> > > +    error (_("Failed to open '%s' for loading execution records: %s"),
> > > +           args, strerror (errno));
> > > +  old_cleanups = make_cleanup (cmd_record_fd_cleanups, &recfd);
> > > +
> > > +  /* Check the magic code.  */
> > > +  record_read_dump (args, recfd, &magic, 4);
> > > +  if (magic != RECORD_FILE_MAGIC)
> > > +    error (_("'%s' is not a valid dump of execution records."), args);
> > > +
> > > +  /* Load the entries in recfd to the record_arch_list_head and
> > > +     record_arch_list_tail.  */
> > > +  record_arch_list_head = NULL;
> > > +  record_arch_list_tail = NULL;
> > Hi Hui,
> >
> > It seems to me that you didn't discard the old recording entries
> > before loading the new ones -- you just added the new ones into
> > the existing list.
> >
> > I don't think that is safe.  We can't be sure that there is
> > any relationship between any existing recording entries and
> > the new entries from the file.  We certainly can't assume
> > that they are consecutive.
> >
> > I think you have to completely clear the record log at this point,
> > before you begin reading new entries from the dump file.
> >
> > What I would suggest is that we add these lines to
> > record_list_release -- and then we can just call that here.
> >
> > @@ -159,6 +159,11 @@ record_list_release (struct record_entry
> >
> >   if (rec != &record_first)
> >     xfree (rec);
> > +
> > +  record_list = &record_first;
> > +  record_arch_list_tail = NULL;
> > +  record_arch_list_tail = NULL;
> > +  record_insn_num = 0;
> >  }