| Message ID | 53875946-83a2-3821-e549-d99bced2e7d8@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 10/14/2016 9:59 AM, Don Breazeal wrote: > 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; > Ping. thanks, --Don
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;