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: Fri, 10 Jun 2016 11:32:20 +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>
* 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;
}
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.
Thanks,
Andrew