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-10 14:25:23 -0700]:

> On 6/10/2016 3:32 AM, Andrew Burgess wrote:
> > * 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;
> >  }
> 
> Hi Andrew,
> Thanks for following up on this.  I like the fact that this patch is
> MI-specific.
> 
> > 
> > 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.
> 
> I had seen the same FAIL with your previous patch in
> gdb.mi/mi-var-create-rtti.exp, and reached the same conclusion.
> 
> I ran the test suite with your new patch and saw one failure that was an
> actual failure:
> 
> FAIL: gdb.mi/mi-var-invalidate.exp: global_simple still alive
> 
> where -var-update expected an empty changelist but didn't get one.
> 
> Long story short, I was able to get rid of that failure by initializing
> innermost_block to the global block instead of the innermost block of
> the current frame, as in the patch below.  With that change the patch
> passes regression testing and my new test.
> 
> The problem occurred in value_of_root_1, where var->root->valid_block
> was no longer NULL:
> 
>  /* Determine whether the variable is still around. */
>  if (var->root->valid_block == NULL || var->root->floating)
>    within_scope = 1;
> 
> If we initialize using the global scope, then varobj_create initializes
> var->root->frame for register varobjs, and value_of_root_1 can still
> determine that global variables are in-scope.
> 
> Here's the updated patch:
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 6f56cba..1e3f192 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_global_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;
>  }
> @@ -2268,7 +2270,9 @@ value_of_root_1 (struct varobj **var_handle)
>    back_to = make_cleanup_restore_current_thread ();
> 
>    /* Determine whether the variable is still around.  */
> -  if (var->root->valid_block == NULL || var->root->floating)
> +  if (var->root->valid_block == NULL
> +      || BLOCK_SUPERBLOCK (var->root->valid_block) == NULL
> +      || var->root->floating)
>      within_scope = 1;
>    else if (var->root->thread_id == 0)
>      {
> 
> Maybe "var->root->valid_block == NULL" is unnecessary above...
> 
> If you think this is a reasonable solution, I'll go ahead and submit a
> new version of the patch including the new test, test updates, etc.

I think this sounds good.

Thanks,
Andrew