From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40737 invoked by alias); 13 Jun 2016 09:05:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 40721 invoked by uid 89); 13 Jun 2016 09:05:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f42.google.com Received: from mail-wm0-f42.google.com (HELO mail-wm0-f42.google.com) (74.125.82.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 13 Jun 2016 09:05:11 +0000 Received: by mail-wm0-f42.google.com with SMTP id k204so69710818wmk.0 for ; Mon, 13 Jun 2016 02:05:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=30BU0xT2JYjAEkZhQQ0JrxsVNGp0ymSZEv4+82U8lKA=; b=lwbAI3XAnOj2GtL11Viqjj1Lfn0LQeXk5YI7S5iKGzVPwzgGV8ZUuQeuZ9qe4+3RtR x25Nr8jaFJm6L4et0uhatmnv7cNx6M5BjCa5Kq4WkMpc6ba3qRTfg901g8/t/oCOPe6C wXn5e9F45tMaFDYlVFY0EFZnZtvcXk7LgwvWBBQuoKxMilw+FOm3r6wirJDffmrjCJyX H1B2uLf4TFLhx8h19LzisK2ETkmoSAjBH0w9kTrN2/JZkToyxsVydKFH84ZaoHnu2Dwd 60akbZUoZTs9mvCYmBwglvRm24NCPcYl9+pc/zCOBS7ps146RP1A73JXsCVO9id8y1jk kdXA== X-Gm-Message-State: ALyK8tKI/yglT3JacRG2Bv6RQXtJ9paFeNkLjD5PhI8WeBcSKAAQU92TsWB0j1H1TSSISg== X-Received: by 10.28.199.75 with SMTP id x72mr9721668wmf.45.1465808707689; Mon, 13 Jun 2016 02:05:07 -0700 (PDT) Received: from localhost (host86-156-236-89.range86-156.btcentralplus.com. [86.156.236.89]) by smtp.gmail.com with ESMTPSA id s1sm26320538wjf.43.2016.06.13.02.05.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Jun 2016 02:05:07 -0700 (PDT) Date: Mon, 13 Jun 2016 09:05:00 -0000 From: Andrew Burgess To: Don Breazeal Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] Fix -var-update for registers in frames 1 and up Message-ID: <20160613090459.GM26734@embecosm.com> References: <1465335417-36881-1-git-send-email-donb@codesourcery.com> <20160608130051.GC26734@embecosm.com> <20160608130856.GD26734@embecosm.com> <5758BCC0.2060700@codesourcery.com> <5759B40D.4010400@codesourcery.com> <20160610103220.GJ26734@embecosm.com> <575B3043.9060608@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <575B3043.9060608@codesourcery.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.6.1 (2016-04-27) X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg00215.txt.bz2 * Don Breazeal [2016-06-10 14:25:23 -0700]: > On 6/10/2016 3:32 AM, Andrew Burgess wrote: > > * Don Breazeal [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 [2016-06-08 14:00:51 +0100]: > >>>> > >>>>> * Don Breazeal [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