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

[Michael Snyder <msnyder@vmware.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;
>>  }
>>
>>
>>