From patchwork Fri Oct 14 16:59:02 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Don Breazeal X-Patchwork-Id: 16510 Received: (qmail 17349 invoked by alias); 14 Oct 2016 16:59:23 -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 17330 invoked by uid 89); 14 Oct 2016 16:59:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=succession, threadid, D*embecosm.com, load_lib X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Oct 2016 16:59:12 +0000 Received: from svr-orw-mbx-02.mgc.mentorg.com ([147.34.90.202]) by relay1.mentorg.com with esmtp id 1bv5p8-0001zY-7L from Don_Breazeal@mentor.com ; Fri, 14 Oct 2016 09:59:10 -0700 Received: from [172.30.0.48] (147.34.91.1) by svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Fri, 14 Oct 2016 09:59:07 -0700 Subject: Re: [PATCH v3] PR mi/20395 - Fix -var-update for registers in frames 1 and up To: Andrew Burgess , "Breazeal, Don" References: <5e305a9a-5582-a2e9-240d-48845ac903d6@redhat.com> <1475614597-109500-1-git-send-email-donb@codesourcery.com> <20161007000520.GT18222@embecosm.com> From: Don Breazeal CC: "gdb-patches@sourceware.org" , "palves@redhat.com" Message-ID: <53875946-83a2-3821-e549-d99bced2e7d8@codesourcery.com> Date: Fri, 14 Oct 2016 09:59:02 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20161007000520.GT18222@embecosm.com> X-ClientProxiedBy: svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) To svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) X-IsSubscribed: yes On 10/6/2016 5:05 PM, Andrew Burgess wrote: > * Don Breazeal [2016-10-04 13:56:37 -0700]: > >> + >> + # Check that the addresses are the same. >> + if {[expr $bpaddr != $pcval]} then { >> + fail "\$pc does not equal address of breakpoint" >> + } else { >> + pass "\$pc equals address of breakpoint" > > This PASS is repeated multiple times and is not unique, which I think > test names are supposed to be. > > Otherwise it looks fine to me, though I'm not a maintainer, so can't > approve the patch :) > > Thanks, > Andrew > Thanks Andrew. I've added a second "with_test_prefix" inside the offending loop to address this. The updated patch follows below, as well as the comments from the previous email detailing the changes prior to this. --Don On 10/4/2016 1:56 PM, Don Breazeal wrote: > I have finally gotten back to this patch and would like to revive it. > > Pedro, you had two concerns about the previous version of this patch. > 1) In varobj.c:varobj_create, the patch changed innermost_block before > the call to the expression parser. You pointed out that > innermost_block is an output parameter. > > 2) The patch had a side-effect of adding a thread-id field to the > output of the -var-update command for global variables. You were > concerned that this wasn't consistent with the documentation and > could potentially break front-ends. > > Here is a new version of the patch. It now assigns innermost_block > in a special case for registers after expression parsing is complete. > Because it does this just for registers, the output of -var-update > for globals is unaffected. > > This does introduce some special case code, which Andrew had wanted > to avoid when he reviewed the original patch. However, with the need > to distinguish between registers and globals wrt setting the block, I > don't see a way around it. > > Updated patch follows. > 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 fixed 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. When a varobj is created by -var-create, varobj->root->frame should be set to specified frame. Then for a subsequent -var-update, varobj.c:check_scope can use varobj->root->frame to select the right frame based on the type of varobj. The problem is that for register expressions 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. The fix is to set innermost block to the global block when creating a register varobj. Then varobj_create sets varobj->root->frame for register varobjs, and varobj_update will select the correct frame for register varobjs. We attempted several alternative solutions: * a special case in varobj_update to select the frame was not ideal because it didn't use the existing mechanisms to select the frame. * setting innermost_block in write_dollar_variable had side-effects in the CLI, breaking 'display' for registers. * setting innermost_block in varobj_create prior to calling the expression parser. This modified an output parameter on input and caused side-effects to -var-update output for globals. Tested on x86_64 Linux native and native-gdbserver, no regressions. gdb/testsuite/ChangeLog: 2016-10-14 Don Breazeal * gdb.mi/mi-frame-regs.exp: New test. gdb/ChangeLog: 2016-10-14 Don Breazeal Andrew Burgess PR mi/20395 * varobj.c (varobj_create): For registers, assign innermost_block to the global block. --- gdb/testsuite/gdb.mi/mi-frame-regs.exp | 184 +++++++++++++++++++++++++++++++++ gdb/varobj.c | 8 ++ 2 files changed, 192 insertions(+) diff --git a/gdb/testsuite/gdb.mi/mi-frame-regs.exp b/gdb/testsuite/gdb.mi/mi-frame-regs.exp new file mode 100644 index 0000000..829bc07 --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp @@ -0,0 +1,184 @@ +# Copyright 1999-2016 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Test essential Machine interface (MI) operations +# +# Verify that -var-update will provide the correct values for floating +# and fixed varobjs that represent the pc register. +# + +load_lib mi-support.exp +set MIFLAGS "-i=mi" + +standard_testfile basics.c + +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \ + executable {debug}] != "" } then { + untested mi-frame-regs.exp + return -1 +} + +# Return the address of the specified breakpoint. + +proc breakpoint_address {bpnum} { + global hex + global expect_out + global mi_gdb_prompt + + send_gdb "info breakpoint $bpnum\n" + gdb_expect { + -re ".*($hex).*$mi_gdb_prompt$" { + return $expect_out(1,string) + } + -re ".*$mi_gdb_prompt$" { + return "" + } + timeout { + return "" + } + } +} + +# Test that a floating varobj representing $pc will provide the +# correct value via -var-update as the program stops at +# breakpoints in different functions. + +proc do_floating_varobj_test {} { + global srcfile + global hex + global expect_out + + gdb_exit + if {[mi_gdb_start]} then { + continue + } + + with_test_prefix "floating" { + mi_run_to_main + + # Create a floating varobj for $pc. + mi_gdb_test "-var-create --thread 1 --frame 0 - @ \$pc" \ + "\\^done,.*value=\"$hex.*" \ + "create floating varobj for pc in frame 0" + + set nframes 4 + for {set i 1} {$i < $nframes} {incr i} { + with_test_prefix "frame $i" { + # Run to a breakpoint in each callee function in succession. + # Note that we can't use mi_runto because we need the + # breakpoint to be persistent, so we can use its address. + set bpnum [expr $i + 1] + mi_create_breakpoint \ + "basics.c:callee$i" \ + "insert breakpoint at basics.c:callee$i" \ + -number $bpnum -func callee$i -file ".*basics.c" + + mi_execute_to "exec-continue" "breakpoint-hit" \ + "callee$i" ".*" ".*${srcfile}" ".*" \ + { "" "disp=\"keep\"" } "breakpoint hit" + + # Get the value of $pc from the floating varobj. + mi_gdb_test "-var-update 1 var1" \ + "\\^done,.*value=\"($hex) .*" \ + "-var-update for frame $i" + set pcval $expect_out(3,string) + + # Get the address of the current breakpoint. + set bpaddr [breakpoint_address $bpnum] + if {$bpaddr == ""} then { + unresolved "get address of breakpoint $bpnum" + return + } + + # Check that the addresses are the same. + if {[expr $bpaddr != $pcval]} then { + fail "\$pc does not equal address of breakpoint" + } else { + pass "\$pc equals address of breakpoint" + } + } + } + } +} + +# Create a varobj for the pc register in each of the frames other +# than frame 0. + +proc var_create_regs {nframes} { + global hex + + for {set i 1} {$i < $nframes} {incr i} { + mi_gdb_test "-var-create --thread 1 --frame $i - \* \$pc" \ + "\\^done,.*value=\"$hex.*" \ + "create varobj for pc in frame $i" + } +} + +# Check that -var-update reports that the value of the pc register +# for each of the frames 1 and above is reported as unchanged. + +proc var_update_regs {nframes} { + + for {set i 1} {$i < $nframes} {incr i} { + mi_gdb_test "-var-update 1 var$i" \ + "\\^done,(changelist=\\\[\\\])" \ + "pc unchanged in -var-update for frame $i" + } +} + +# Test that fixed varobjs representing $pc in different stack frames +# will provide the correct value via -var-update after the program +# counter changes (without substantially changing the stack). + +proc do_fixed_varobj_test {} { + global srcfile + + gdb_exit + if {[mi_gdb_start]} then { + continue + } + + with_test_prefix "fixed" { + mi_run_to_main + + # Run to the function 'callee3' so we have several frames. + mi_create_breakpoint "basics.c:callee3" \ + "insert breakpoint at basics.c:callee3" \ + -number 2 -func callee3 -file ".*basics.c" + + mi_execute_to "exec-continue" "breakpoint-hit" \ + "callee3" ".*" ".*${srcfile}" ".*" \ + { "" "disp=\"keep\"" } "breakpoint hit" + + # At the breakpoint in callee3 there are 4 frames. Create a + # varobj for the pc in each of frames 1 and above. + set nframes 4 + var_create_regs $nframes + + # Step one instruction to change the program counter. + mi_execute_to "exec-next-instruction" "end-stepping-range" \ + "callee3" ".*" ".*${srcfile}" ".*" "" \ + "next instruction in callee3" + + # Check that -var-update reports that the values are unchanged. + var_update_regs $nframes + } +} + +do_fixed_varobj_test +do_floating_varobj_test + +mi_gdb_exit +return 0 diff --git a/gdb/varobj.c b/gdb/varobj.c index fb1349a..80738af 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -348,6 +348,14 @@ varobj_create (char *objname, " as an expression.\n"); return NULL; } + else if (var->root->exp->elts[0].opcode == OP_REGISTER) + { + /* Force the scope of a register varobj to be the global + block. This allows it to be associated with a frame + and a thread. */ + gdb_assert (innermost_block == NULL); + innermost_block = block_global_block (block); + } var->format = variable_default_display (var); var->root->valid_block = innermost_block;