From patchwork Fri Jun 10 10:32:20 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 12941 Received: (qmail 57137 invoked by alias); 10 Jun 2016 10:32:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 57125 invoked by uid 89); 10 Jun 2016 10:32:42 -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 autolearn=ham version=3.3.2 spammy=relate, threadid, H*r:sk:host81-, Hx-spam-relays-external:sk:host81- X-HELO: mail-wm0-f44.google.com Received: from mail-wm0-f44.google.com (HELO mail-wm0-f44.google.com) (74.125.82.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 10 Jun 2016 10:32:32 +0000 Received: by mail-wm0-f44.google.com with SMTP id r190so2178556wmr.0 for ; Fri, 10 Jun 2016 03:32:31 -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=OLQLKr9nE/HjkNu4P/GN7e+fwZGGPPcOCLi5e1jHKio=; b=M2kWEafMry5K9LkXBIsz7/9mZBPgdHTUzjfHILKg7rtf5cW/aOXIPfYFR7pQ9J1dd1 +g7Gf/b5g8UP25GzW7Qav2074CfXKCWVGOfj60pUfwCnCH5apapH3MpSQ9Y5X8vTIw6o cZDzUT1QCcw3wcE9BTofBmnpXWcLTrI8b16COSVaAhx2cjyQ8ljXhkWlyeVSdl/5uH6y qRLyMEkNE3e2q+vuZKwZcvhsE9ZWSDjnfPzp4bZ9jNOAX33Lg5zZ30IWUUcfbAmyIq/U anx9FJeP4PNkNqvcyJ7CaNScqoZOwILSC7kNXxzf3mlEPb/Xhlzd1bQz9Lm1rS/hIWeT oBMQ== X-Gm-Message-State: ALyK8tLG8y6LZqaDF7jFRp9T4KADB1+8vVbsna2JfetKDiI3vo+5M193JZJT2AWrzFIEiQ== X-Received: by 10.28.146.73 with SMTP id u70mr2157008wmd.42.1465554748729; Fri, 10 Jun 2016 03:32:28 -0700 (PDT) Received: from localhost (host81-147-175-48.range81-147.btcentralplus.com. [81.147.175.48]) by smtp.gmail.com with ESMTPSA id l9sm11657425wjm.0.2016.06.10.03.32.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Jun 2016 03:32:28 -0700 (PDT) Date: Fri, 10 Jun 2016 11:32:20 +0100 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: <20160610103220.GJ26734@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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5759B40D.4010400@codesourcery.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.6.1 (2016-04-27) X-IsSubscribed: yes * 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: 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 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; }