Re: [PATCH] Variable objects in multi-threaded programs

[Vladimir Prus <vladimir@codesourcery.com>]

On Wednesday 30 January 2008 10:57:29 Nick Roberts wrote:
>  > >  > > +static void
>  > >  > > +check_scope (struct varobj *var, int* scope)
>  > >  > > +{
>  > >  > 
>  > >  > This function needs a comment. 'check_scope', in
>  > >  > itself, can do just about anything.
>  > > 
>  > > This is a small function and I think it speaks for itself.
>  > 
>  > Not in my opinion. 'check_scope' can mean anything.
> 
> The function name doesn't speak for itself, but being only about twelve lines
> of code, it is easy to see what it does..

It might be easy to see what the function body does. But I don't know
what's the intended usage of the function is, and when the preconditions are
(for example, calling this function when varobj has no associated frame is
probably not going to work).

For example, here's what I'd write as comment:

	/* If the frame associated with this varobj exists,
           and the address in that frame is inside varobj's block,
	   switch to the frame and return 1.  Otherwise, return 0.
	   This function should be called only for variable object
	   that are associated with frame and block, and only in
           the thread associated with the frame.  */

These are 6 lines of english text, and one can understand everything
he needs to know about check_scope, without looking at 12 lines
of body, and without knowing anything about functions called in
that body.

> 
>  > > I don't 
>  > > know if GNU Coding standards dictate how much commenting there should be but
>  > > I think that in varobj.c there is too much and it makes the code harder to
>  > > read.
>  > 
>  > I disagree -- practice shows that the exact semantics of varobj
>  > logic is still not 100% clear.
> 
> But it looks like the coding standards allow us to choose our own style.

Yes, the GNU coding standards mostly have to do with brace placement, that's
why I don't understand why you use it as reference in this case.

I can only repeat that in practice, varobj.c proves hard to understand, which suggest
it needs more comments, not less.

>  > >  > > +        saved_frame_id = get_frame_id (get_selected_frame (NULL));
>  > >  > > +        old_cleanups = make_cleanup_restore_current_thread (inferior_ptid,
>  > >  > >				saved_frame_id); 
>  > >  > 
>  > >  > Presumably, we can move this to the top-level of c_value_of_root, and then
>  > >  > get rid of save/restore of selected frame in varobj_update?
>  > > 
>  > > No.  This is only necessary in the multi-threaded case.
>  > 
>  > make_cleanup_restore_current_thread arranges for two things to
>  > be restored:
>  >    - current thread
>  >    - selected frame
>  > 
>  > We now always save/restore selected frame in varobj_update. And
>  > changing/saving/restoring current thread is extremely fast. So,
>  > why not just save both thread and frame in all cases, and simplify
>  > the logic.
> 
> Then we presumably could remove the calls after value_of_root:
> 
>       /* Restore selected frame.  */
>       fi = frame_find_by_id (old_fid);
>       if (fi)
> 	select_frame (fi);
> 
> but I have no idea how fast do_restore_current_thread_cleanup is.  Indirectly,
> it calls functions like reinit_frame_cache.

Well:

	void
	switch_to_thread (ptid_t ptid)
	{
	  if (ptid_equal (ptid, inferior_ptid))
	    return;

	  inferior_ptid = ptid;
	  reinit_frame_cache ();
	  registers_changed ();
	  stop_pc = read_pc ();
	}

So in the case of non-threaded program, we exit early and pay no price.

- Volodya