Re: [RFA] Submit process record and replay fourth time, 0/8

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 15 April 2009 17:56:34, Hui Zhu wrote:

> >> +struct cleanup *
> >> +record_gdb_operation_disable_set (void)
> >> +{
> >> + ?struct cleanup *old_cleanups = NULL;
> >> +
> >> + ?if (!record_gdb_operation_disable)
> >> + ? ?{
> >> + ? ? ?old_cleanups =
> >> + ? ? ? ?make_cleanup_restore_integer (&record_gdb_operation_disable);
> >> + ? ? ?record_gdb_operation_disable = 1;
> >> + ? ?}
> >> +
> >> + ?return 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. ?It returns NULL when for the
> > first cleanup put in the chain. ?See make_my_cleanup2.
> >
> 
>   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 = *pmy_chain) != old_chain)
>     {
>       *pmy_chain = 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 = 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)
> >> +{
> >> + ?if (execution_direction == EXEC_REVERSE)
> >> + ? ?{
> >> + ? ? ?if (record_list->next)
> >> + ? ? record_list = record_list->next;
> >> + ? ?}
> >> + ?else
> >> + ? ?record_list = record_list->prev;
> >> +}
> >
> >> +static ptid_t
> >> +record_wait (struct target_ops *ops,
> >> + ? ? ? ? ? ? ?ptid_t ptid, struct target_waitstatus *status)
> > (...)
> >> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
> >
> > This cleanup looks suspiciously broken to me. ?It 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. ?:P
> 
> 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?
 
> +static void
> +record_wait_cleanups (void *ignore)
> +{
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      if (record_list->next)
         ^^^^^^^^^^^^^^^^^^^^^^
> +     record_list = record_list->next;
> +    }
> +  else
> +    record_list = 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 == target-endianess, that the registers are 32-bit, and
> > probably that signed<->unsigned bit representation is equal. ?Is
> > this what you want? ?When 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 future.

Ok.

-- 
Pedro Alves