Re: [PATCH] Fix -var-update for registers in frames 1 and up

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Don Breazeal <donb@codesourcery.com> [2016-06-09 11:23:09 -0700]:

> On 6/8/2016 5:48 PM, Breazeal, Don wrote:
> > On 6/8/2016 6:08 AM, Andrew Burgess wrote:
> >> * Andrew Burgess <andrew.burgess@embecosm.com> [2016-06-08 14:00:51 +0100]:
> >>
> >>> * Don Breazeal <donb@codesourcery.com> [2016-06-07 14:36:57 -0700]:
> >>>
> >>>> This patch fixes a problem with using the MI -var-update command
> >>>> to access the values of registers in frames other than the current
> >>>> frame.  The patch includes a test that demonstrates the problem:
> >>>>
> >>>> * run so there are several frames on the stack
> >>>> * create a varobj for $pc in each frame, #'s 1 and above
> >>>> * step one instruction, to modify the value of $pc
> >>>> * call -var-update for each of the previously created varobjs
> >>>>   to verify that they are not reported as having changed.
> >>>>
> >>>> Without the patch, the -var-update command reported that $pc for all
> >>>> frames 1 and above had changed to the value of $pc in frame 0.
> >>>>
> >>>> The -var-create command takes a '--frame' argument and uses that
> >>>> to select the frame for retrieving the register value.  But -var-update
> >>>> has no such argument, and previously didn't do anything to select the
> >>>> frame, so for frames other than frame 0 it returned the value of the
> >>>> register for frame 0, instead of reporting the value as unchanged.
> >>>
> >>> This shouldn't need special handling for register varobj values, if I
> >>> create a varobj watching value 'foo' in frame 1, but have a local
> >>> 'foo' in frame 0, a change in frame 0 'foo' will not trigger a change
> >>> for frame 1's 'foo' varobj.
> >>>
> >>> The magic is actually in varobj.c:check_scope, which makes use of the
> >>> varobj->root->frame to select the right frame based on the type of the
> >>> varobj, this is setup at varobj creation time.
> >>>
> >>> The problem, is that for register expressions the varobj->root->frame
> >>> is not set correctly.  This frame tracking is done using the global
> >>> 'innermost_block' which is set in the various parser files (for
> >>> example c-exp.y), however, this is not set for register expressions.
> >>> I think that we probably should be doing this in
> >>> write_dollar_variable.
> > 
> > Andrew,
> > Thanks for explaining.  I had followed innermost_block quite a bit
> > in my debugging, but somehow convinced myself that wasn't the solution.
> > 
> >>
> >> Something like the following (untested):
> >>
> >> diff --git a/gdb/parse.c b/gdb/parse.c
> >> index 2b00708..224c022 100644
> >> --- a/gdb/parse.c
> >> +++ b/gdb/parse.c
> >> @@ -705,6 +705,10 @@ handle_register:
> >>    str.ptr++;
> >>    write_exp_string (ps, str);
> >>    write_exp_elt_opcode (ps, OP_REGISTER);
> >> +  if (innermost_block == NULL
> >> +      || contained_in (expression_context_block,
> >> +		       innermost_block))
> >> +    innermost_block = expression_context_block;
> >>    return;
> >>  }
> > 
> > Your solution works for both the fixed a floating varobj cases.
> > I've extended my new test to cover the floating case.  Unfortunately
> > in the full test suite I've run into some unexpected side-effects
> > that need further investigation.
> 
> I haven't gone through all the failures, but I have found the cause of
> one of the side-effects of this change.  In the CLI, the 'display'
> command depends on parsing a register expression to *not* set
> innermost_block.  If the expression has a block, it has to be in-scope
> or display will skip it rather than evaluating the expression.

OK, so dollar variables shouldn't force any kind of scope
requirement.

>                                                                 See
> printcmd.c:do_one_display, the handling of d->block.  It basically does:
> 
>          d->exp = parse_expression (d->exp_string);
>          d->block = innermost_block;
> ---snip---
>    if (d->block)
>     {
>       if (d->pspace == current_program_space)
>         within_current_scope = contained_in (get_selected_block (0),
> d->block);
> ---snip---
>     }
>   else
>     within_current_scope = 1;
>   if (!within_current_scope)
>     return;
> 
> It seems like we will need a special case someplace.  In do_one_display
> we could NULL out d->block for register expressions, or in varobj_update
> we could expand on the original patch to properly handle floating
> varobjs.

I see it slightly different.  We have a mechanism that works fine, so
long as the var->root->frame is set up correctly on the varobj.  As a
minimum, any special case handling should be moved into varobj_create
and should focus on fixing var->root->frame.

But, I think we can do better.

Before expression parsing we set 'innermost_block = NULL;' then we
parse the expression and 'innermost_block' might have changed to
reflect a need for a "more inner" block.  The initial setting to NULL
as far as I can tell is basically saying, start with no scoping
requirement, and let the expression tell us what it needs.

But, what if instead of starting at NULL, we started at a block/frame
based on the type of varobj being created.   The expression parsing
can still force a "more inner" block requirement, but the default
would no longer be NULL.  It turns out we already have a variable
'block' inside varobj_create that is initialised based on the frame
that is, in turn, initialised based on the type of varobj being
created.  Here's a new patch:

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 6f56cba..f89ae80 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -323,7 +323,7 @@ varobj_create (char *objname,
 	}
 
       p = expression;
-      innermost_block = NULL;
+      innermost_block = block;
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
@@ -2103,6 +2103,8 @@ new_root_variable (void)
   var->root->floating = 0;
   var->root->rootvar = NULL;
   var->root->is_valid = 1;
+  var->root->thread_id = 0;
+  var->root->next = NULL;
 
   return var;
 }

I've actually tested this one, and I see 7 failures.  I've not looked
at them all yet, but the ones I have looked at all relate to the
output of various varobj commands now including a thread-id field when
they didn't before.  This is a knock on from the varobj having a frame
when it previously didn't.

The thing is, if we ask for a particular expression in a particular
frame, doesn't that imply a particular thread?  We probably need to
review the regressions, but I'm tempted, initially, to say that the
new thread-id is actually the right behaviour, and the test results
should be updated.


Thanks,
Andrew