Re: Thread bound variable objects [was: Re: MI non-stop mode spec]

[Vladimir Prus <vladimir@codesourcery.com>]

On Monday 24 March 2008 09:57:24 Nick Roberts wrote:
>  > > OK, it should be:
>  > > 
>  > >   +  if (old_cleanups != NULL)
>  > >   +    do_cleanups (old_cleanups);
>  > 
>  > I think that's also wrong. In the event that no cleanups were installed
>  > before calling this function, this code will fail to run the
>  > cleanups installed by this function. In the event that a cleanup
>  > should be really installed conditionally, the right code is:
>  > 
>  > 	struct cleanups *back_to  = make_cleanup (null_cleanup, NULL);
>  > 
>  > 	if (...)
>  > 		make_cleanup ();
>  > 
>  > 	do_cleanups (back_to);
> 
> I don't really see that: if nothing is added to cleanup_chain then no cleanups
> are done.

Suppose that no cleanups were installed before this function is called. Then, the
first call to make_cleanup will return NULL, which will be stored in 'old_cleanups'.
Then, at the end of the function you will execute:

	if (old_cleanups != NULL)
	   do_cleanups (old_cleanups);

Here, old_cleanups is NULL, so cleanups added in the function are not run.

>  > > I see now from the ChangeLog that you've committed your own change without
>  > > posting to the list first or explaining what it does.
>  > 
>  > Sorry, no:
>  > 1. I've checked in two changes, not one.
>  > 2. Both are posted to gdb-patches.
> 
> OK, my mistake.  I saw the first one.
> 
>  > 3. Both are general cleanups, and don't implement anything that your patch
>  > tries to implement.
> 
> It's just that the second one includes changes in and overlaps with my patch so
> I thought it was part of that thread.  I'm not sure where that leaves things
> now.

I've started looking at your patch, and saw various cleanups that can
be done first, which I've done. I'll now get back to your patch proper, and hopefully
will get it in today.

>  > > My patch does two things:
>  > > 
>  > > 1) It stops a variable object from being considered automatically out of
>  > >    scope when the selected thread changes.
>  > > 2) It associates a thread-id field with the variable object so that the
>  > >    front end can organise the display of watch expressions accordingly.
>  > > 
>  > > AFAICS your patch does neither of these.  Could you please say what it
>  > > does do?
>  > 
>  > Please see my gdb-patches posts.
> 
> With regard to the second patch, why you are using
> make_cleanup_restore_current_thread when you aren't switching the thread in the
> first place?

Because make_cleanup_restore_current_thread is handy pre-existing solution 
that will restore both thread and frame, and won't cause any harm if the 
thread is not actually changed. I probably could have introduced another 
cleanup to restore just frame, but that will be less safe, and only 
insignificantly faster.

- Volodya