From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22858 invoked by alias); 7 Aug 2009 03:29:32 -0000 Received: (qmail 22848 invoked by uid 22791); 7 Aug 2009 03:29:31 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Aug 2009 03:29:26 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id CEA823C000; Thu, 6 Aug 2009 20:29:24 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost4.vmware.com (Postfix) with ESMTP id C2CB6C9A3C; Thu, 6 Aug 2009 20:29:24 -0700 (PDT) Message-ID: <4A7B9F49.9030202@vmware.com> Date: Fri, 07 Aug 2009 03:34:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Hui Zhu CC: Eli Zaretskii , "gdb-patches@sourceware.org" Subject: Re: [RFA/RFC] Add dump and load command to process record and replay References: <83ws5koinl.fsf@gnu.org> <83my6fo2pa.fsf@gnu.org> <4A78935B.5030508@vmware.com> <4A79F802.4060102@vmware.com> <83ab2docqi.fsf@gnu.org> <4A7B99B3.40407@vmware.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-08/txt/msg00099.txt.bz2 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 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; >> } >> >> >>