This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix -var-update for registers in frames 1 and up
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: Don Breazeal <donb at codesourcery dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Mon, 13 Jun 2016 10:04:59 +0100
- Subject: Re: [PATCH] Fix -var-update for registers in frames 1 and up
- Authentication-results: sourceware.org; auth=none
- References: <1465335417-36881-1-git-send-email-donb at codesourcery dot com> <20160608130051 dot GC26734 at embecosm dot com> <20160608130856 dot GD26734 at embecosm dot com> <5758BCC0 dot 2060700 at codesourcery dot com> <5759B40D dot 4010400 at codesourcery dot com> <20160610103220 dot GJ26734 at embecosm dot com> <575B3043 dot 9060608 at codesourcery 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