From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17795 invoked by alias); 21 Apr 2009 13:31:13 -0000 Received: (qmail 17753 invoked by uid 22791); 21 Apr 2009 13:31:09 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 21 Apr 2009 13:31:02 +0000 Received: (qmail 27299 invoked from network); 21 Apr 2009 13:30:59 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 21 Apr 2009 13:30:59 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] Submit process record and replay fourth time, 0/8 Date: Tue, 21 Apr 2009 13:31:00 -0000 User-Agent: KMail/1.9.10 Cc: Hui Zhu , Mark Kettenis , Marc Khouzam , Michael Snyder , Thiago Jung Bauermann , Eli Zaretskii , paawan1982@yahoo.com References: <200904021526.53212.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200904211431.29470.pedro@codesourcery.com> 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-04/txt/msg00561.txt.bz2 On Wednesday 15 April 2009 17:56:34, Hui Zhu wrote: > >> +struct cleanup * > >> +record_gdb_operation_disable_set (void) > >> +{ > >> + =A0struct cleanup *old_cleanups =3D NULL; > >> + > >> + =A0if (!record_gdb_operation_disable) > >> + =A0 =A0{ > >> + =A0 =A0 =A0old_cleanups =3D > >> + =A0 =A0 =A0 =A0make_cleanup_restore_integer (&record_gdb_operation_d= isable); > >> + =A0 =A0 =A0record_gdb_operation_disable =3D 1; > >> + =A0 =A0} > >> + > >> + =A0return old_cleanups; > >> +} > > > > This returns a NULL cleanup if record_gdb_operation_disable is > > already set, but make_cleanup also returns a legitimate > > NULL, and it is not an error. =A0It returns NULL when for the > > first cleanup put in the chain. =A0See make_my_cleanup2. > > >=20 > if (set_cleanups) > do_cleanups (set_cleanups); > void > do_cleanups (struct cleanup *old_chain) > { > do_my_cleanups (&cleanup_chain, old_chain); > } > static void > do_my_cleanups (struct cleanup **pmy_chain, > struct cleanup *old_chain) > { > struct cleanup *ptr; > while ((ptr =3D *pmy_chain) !=3D old_chain) > { > *pmy_chain =3D ptr->next; /* Do this first incase recursion */ > (*ptr->function) (ptr->arg); > if (ptr->free_arg) > (*ptr->free_arg) (ptr->arg); > xfree (ptr); > } > } > NULL will make all the chain clean. Yes, and that's the problem with your code. You should *not* treat a NULL cleanup any differently from any other cleanup value. If there was no cleanup yet installed in the chain when you called make_cleanup, make_cleanup will return NULL. If later, you check on the result of make_cleanup old_chain =3D make_cleanup (foo, NULL); ^^^^^^^^^ ^ - this will return NULL if there's nothing else in the chain. But, after the call there's a new cleanup installed (your `foo' cleanup), that you do want to run. if (old_chain) do_cleanups (old_chain); ^^^^^^^^^^^^^ ^ - so this is wrong, because old_chain may be NULL, and you still wanted the do_cleanups call to happen. > >> +static void > >> +record_wait_cleanups (void *ignore) > >> +{ > >> + =A0if (execution_direction =3D=3D EXEC_REVERSE) > >> + =A0 =A0{ > >> + =A0 =A0 =A0if (record_list->next) > >> + =A0 =A0 record_list =3D record_list->next; > >> + =A0 =A0} > >> + =A0else > >> + =A0 =A0record_list =3D record_list->prev; > >> +} > > > >> +static ptid_t > >> +record_wait (struct target_ops *ops, > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0ptid_t ptid, struct target_waitstatus *st= atus) > > (...) > >> +struct cleanup *old_cleanups =3D make_cleanup (record_wait_cleanups, = 0); > > > > This cleanup looks suspiciously broken to me. =A0It appears that > > is will do weird things depending on when an exception is thrown. > > But then again, record_wait's nesting and use of goto's makes > > my eyes bleed. =A0:P >=20 > This cleanup will make record_list point to the record entry that > execution before this record entry because set this record entry get > fail. > This part is not very easy to make clear. I make clear it use a lot > of time. :P It looks broken, e.g., because if the cleanup runs before adding a new entry to the list, you'd undo the wrong thing. You needed a conditional on 'record_list->next' being not-NULL --- if the cleanup is always safe and correct to run, why did you need it? =20 > +static void > +record_wait_cleanups (void *ignore) > +{ > + if (execution_direction =3D=3D EXEC_REVERSE) > + { > + if (record_list->next) ^^^^^^^^^^^^^^^^^^^^^^ > + record_list =3D record_list->next; > + } > + else > + record_list =3D record_list->prev; > +} I looks like it would be better to build the record entry, and only when everything is fine and validated, you'd add it to the record list. That way, you wouldn't need to (dangerously) undo a bad list entry. But I'm not going to make a fuss about this. Let's keep it that way for now if you'd like. > > This (and many more similar instances) is assuming > > host-endianness =3D=3D target-endianess, that the registers are 32-bit,= and > > probably that signed<->unsigned bit representation is equal. =A0Is > > this what you want? =A0When you get to 64-bit, most of this will break, > > it seems. > For now, record just support i386 arch. Please let us keep it to the fut= ure. Ok. --=20 Pedro Alves