On Thursday 05 June 2008 19:41:11 Daniel Jacobowitz wrote:
> On Sun, May 04, 2008 at 12:25:54PM +0400, Vladimir Prus wrote:
> > Are those two new patches, together with the previously posted one
> > (changing stop_observer not to always fire) OK?
> 
> For this patch, if an error occurs during proceed (i.e. if the
> cleanup restoring suppress_normal_stop_observer is run), will the
> normal_stop observer ever be called?  Is that a problem?

You mean this block:

    old_cleanups2 = make_cleanup_restore_integer 
      (&suppress_normal_stop_observer, 1);
     proceed (real_pc, TARGET_SIGNAL_0, 0);
    do_cleanups (old_cleanups2);

If proceed throws, before calling normal_stop, we'll get back to event loop,
and run cleanup. We won't call the observer. It's an issue if we've printed
"*running" and thrown after after. However, it's the issue we have now, as
well -- we print ^running even before calling proceed, and if something later
throws, we'll never print *stopped. Possible solutions are:
- Require that frontend refresh thread state on ^error
- Emit *stopped if exception is thrown (this requires checking that the
target is actually stopped, if exception is thrown).

> > +   If the argument is pointer to allocated memory, then you need to
> 
> is a pointer
> 
> >  struct cleanup *
> > -make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
> > -		 void *arg)
> > +make_cleanup_restore_integer (int *variable, int value)
> > +{
> > +  struct restore_integer_closure *c =
> > +    xmalloc (sizeof (struct restore_integer_closure));
> > +  struct cleanup *cleanup = make_cleanup (restore_integer, (void *) c);
> > +  c->variable = variable;
> > +  c->value = *variable;    
> > +  *variable = value;
> > +  cleanup_chain->free_arg = xfree;
> > +  return cleanup;
> > +}
> 
> Could you use make_my_cleanup2 here to avoid poking around in
> cleanup_chain, please?

Done.

> 
> Also, the only thing value is used for is to set *variable.  I
> suggest not setting the variable in a function named
> "make_cleanup_restore_integer", which doesn't say anything about
> setting.  So I would prefer this:
> 
>   old_cleanup = make_cleanup_restore_integer (some_var);
>   some_var = 0;

Done.

> 
> > 	* infrun.c (finish_command): Don't pass cleanup
> > 	to continuation.
> > 	(finish_command_continuation): Don't grab cleanup from
> > 	the passed data, as we don't use, and cannot, use it anyway.
> 
> OK.

Thanks, checked in. Since there were some adjustment, I attach
the final versions of the patches as checked in.

- Volodya