diff mbox

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

Message ID 20160608130856.GD26734@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess June 8, 2016, 1:08 p.m. UTC
* 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.

Something like the following (untested):

Thanks,
Andrew

Comments

Don Breazeal June 9, 2016, 12:48 a.m. UTC | #1
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.

Hopefully an updated patch will be forthcoming soon.
--Don
Don Breazeal June 9, 2016, 6:23 p.m. UTC | #2
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
diff mbox

Patch

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;
 }