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

[Hui Zhu <teawater at gmail dot com>]

I think a warning is clear to most of people.

And when he get this warning.  He can delete the record list and load
again.  He will lost nothing.

If we delete the old record list, maybe he still need old record.  He
will lost something.

Hui

On Fri, Aug 7, 2009 at 11:28, Michael Snyder<msnyder@vmware.com> wrote:
> 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;
>>> ?}
>>>
>>>
>>>
>
>