This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] PR mi/20395 - Fix -var-update for registers in frames 1 and up
- From: Don Breazeal <donb at codesourcery dot com>
- To: Andrew Burgess <andrew dot burgess at embecosm dot com>, "Breazeal, Don" <Don_Breazeal at mentor dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "palves at redhat dot com" <palves at redhat dot com>
- Date: Fri, 14 Oct 2016 09:59:02 -0700
- Subject: Re: [PATCH v3] PR mi/20395 - Fix -var-update for registers in frames 1 and up
- Authentication-results: sourceware.org; auth=none
- References: <5e305a9a-5582-a2e9-240d-48845ac903d6@redhat.com> <1475614597-109500-1-git-send-email-donb@codesourcery.com> <20161007000520.GT18222@embecosm.com>
On 10/6/2016 5:05 PM, Andrew Burgess wrote:
> * Don Breazeal <donb@codesourcery.com> [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 <donb@codesourcery.com>
* gdb.mi/mi-frame-regs.exp: New test.
gdb/ChangeLog:
2016-10-14 Don Breazeal <donb@codesourcery.com>
Andrew Burgess <andrew.burgess@embecosm.com>
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 <http://www.gnu.org/licenses/>.
+
+# 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;
--
2.8.1