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

[Don Breazeal <donb@codesourcery.com>]

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.  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.  Perhaps since the problem under consideration is only in MI,
making the MI-specific change in varobj_update would minimize
side-effects in the CLI.
WDYT?

I will continue investigating the failures to see if there is anything
else there.
thanks
--Don