Fix -var-update for registers in frames 1 and up

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:


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.

thanks
--Don

* 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