diff mbox

[v3] PR mi/20395 - Fix -var-update for registers in frames 1 and up

Message ID 53875946-83a2-3821-e549-d99bced2e7d8@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Oct. 14, 2016, 4:59 p.m. UTC
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(+)

Comments

Don Breazeal Oct. 26, 2016, 6:04 p.m. UTC | #1
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 mbox

Patch

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;