[PING^4] [PATCH v2] Fix -var-update for registers in frames 1 and up

[Don Breazeal <donb at codesourcery dot com>]

ping

On 7/21/2016 9:59 AM, Don Breazeal wrote:
> Hi, see below,
> 
> On 7/20/2016 2:06 PM, Don Breazeal wrote:
>> ping
>>
>> On 7/11/2016 7:48 AM, Don Breazeal wrote:
>>>
>>> ping
>>>
>>> On 6/20/2016 8:31 AM, Don Breazeal wrote:
>>>> On 6/13/2016 2:54 PM, Don Breazeal wrote:
>>>>> This is an updated version of a patch to fix access to registers
>>>>> for frames other than the current frame via var-update.  The piece
>>>>> of the patch affecting GDB proper has been completely re-written.
>>>>> It changes the initialization of varobj->root->frame in varobj_create
>>>>> so that existing mechanisms (with minimal changes) select the correct
>>>>> frame and provide correct register values from -var-update.
>>>>>
>>>>> In addition, the new test for this functionality has been expanded
>>>>> to cover an analogous case using floating variable objects, and
>>>>> several tests have been updated to account for the fact that with
>>>>> the new initialization, -var-create and -var-list-children now
>>>>> provide a thread-id field where previously they didn't.
>>>>>
>>>>> Thanks
>>>>> --Don
>>>>>
>>>>> ------------
>>>>> 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 change the initialization of innermost_block in
>>>>> varobj_create from NULL to the global block, as returned by
>>>>> block_global_block.  Then varobj_create sets varobj->root->frame
>>>>> for register varobjs, and value_of_root_1 can check for the
>>>>> global block when determining whether the variable is in-scope
>>>>> and select the frame appropriately.
>>>>>
>>>>> A side-effect of this change is that for register varobjs and some
>>>>> global variable varobjs, the output of -var-create now includes the
>>>>> thread-id field.  The rationale for this is as follow: if we ask for a
>>>>> particular expression in a particular frame, that implies a particular
>>>>> thread.  Thus including the thread-id is correct behavior, and the
>>>>> test results have been updated accordingly.
>>>>>
>>>>> In addition there is a new test, gdb.mi/mi-frame-regs.exp, which
>>>>> performs the test described in the bullet list above as well as a
>>>>> similar test using a floating variable object to represent $pc.
>>>>>
>>>>> We attempted a couple 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 that would have required special case code.
>>>>>
>>>>> Tested on x86_64 Linux native, no regressions.
>>>>>
>>>>> gdb/testsuite/ChangeLog:
>>>>> 2016-06-13  Don Breazeal  <dbreazea@my.domain.org>
>>>>>
>>>>> 	* gdb.ada/mi_interface.exp: Add thread-id field to expected
>>>>> 	output of -var-create and -var-list-children.
>>>>> 	* gdb.ada/mi_var_array.exp: Add thread-id field to expected
>>>>> 	output of -var-list-children.
>>>>> 	* gdb.mi/mi-break.exp (test_error): Add thread-id field to
>>>>> 	expected output of -var-create.
>>>>> 	* gdb.mi/mi-frame-regs.exp: New test script.
>>>>> 	* gdb.mi/mi-var-cmd.exp: Add thread-id field to expected
>>>>> 	output of -var-create.
>>>>> 	* gdb.mi/mi-var-create-rtti.exp: Likewise.
>>>>>
>>>>> gdb/ChangeLog:
>>>>> 2016-06-13  Don Breazeal  <donb@codesourcery.com>
>>>>> 	    Andrew Burgess <andrew.burgess@embecosm.com>
>>>>>
>>>>> 	* varobj.c (varobj_create): Initialize innermost_block to
>>>>> 	the global block instead of NULL.
>>>>> 	(new_root_variable): Initialize the thread_id and next
>>>>> 	fields.
>>>>> 	(value_of_root_1): Set within_scope if the variable's
>>>>> 	valid_block field is the global block.
>>>>>
>>>>> ---
>>>>>  gdb/testsuite/gdb.ada/mi_interface.exp      |   4 +-
>>>>>  gdb/testsuite/gdb.ada/mi_var_array.exp      |   2 +-
>>>>>  gdb/testsuite/gdb.mi/mi-break.exp           |   2 +-
>>>>>  gdb/testsuite/gdb.mi/mi-frame-regs.exp      | 183 ++++++++++++++++++++++++++++
>>>>>  gdb/testsuite/gdb.mi/mi-var-cmd.exp         |   4 +-
>>>>>  gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |   2 +-
>>>>>  gdb/varobj.c                                |   8 +-
>>>>>  7 files changed, 196 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/gdb/testsuite/gdb.ada/mi_interface.exp b/gdb/testsuite/gdb.ada/mi_interface.exp
>>>>> index 6000ec8..b948cd5 100644
>>>>> --- a/gdb/testsuite/gdb.ada/mi_interface.exp
>>>>> +++ b/gdb/testsuite/gdb.ada/mi_interface.exp
>>>>> @@ -44,9 +44,9 @@ mi_continue_to_line \
>>>>>      "stop at start of main Ada procedure"
>>>>>  
>>>>>  mi_gdb_test "-var-create ggg1 * ggg1" \
>>>>> -    "\\^done,name=\"ggg1\",numchild=\"1\",value=\"{...}\",type=\"<ref> pck.gadatatype\",has_more=\"0\"" \
>>>>> +    "\\^done,name=\"ggg1\",numchild=\"1\",value=\"{...}\",type=\"<ref> pck.gadatatype\",thread-id=\"1\",has_more=\"0\"" \
>>>>>      "Create ggg1 varobj"
>>>>>  
>>>>>  mi_gdb_test "-var-list-children 1 ggg1" \
>>>>> -    "\\^done,numchild=\"1\",children=\\\[child={name=\"ggg1.i\",exp=\"i\",numchild=\"0\",value=\"42\",type=\"integer\"}\\\],has_more=\"0\"" \
>>>>> +    "\\^done,numchild=\"1\",children=\\\[child={name=\"ggg1.i\",exp=\"i\",numchild=\"0\",value=\"42\",type=\"integer\",thread-id=\"1\"}\\\],has_more=\"0\"" \
>>>>>      "list ggg1's children"
>>>>> diff --git a/gdb/testsuite/gdb.ada/mi_var_array.exp b/gdb/testsuite/gdb.ada/mi_var_array.exp
>>>>> index c648e7e..c02d4c9 100644
>>>>> --- a/gdb/testsuite/gdb.ada/mi_var_array.exp
>>>>> +++ b/gdb/testsuite/gdb.ada/mi_var_array.exp
>>>>> @@ -48,5 +48,5 @@ mi_gdb_test "-var-create vta * vta" \
>>>>>      "Create bt varobj"
>>>>>  
>>>>>  mi_gdb_test "-var-list-children vta" \
>>>>> -    "\\^done,numchild=\"2\",children=\\\[child={name=\"vta.n\",exp=\"n\",numchild=\"0\",type=\"bar\\.int\"},child={name=\"vta.f\",exp=\"f\",numchild=\"0\",type=\"array \\(1 .. n\\) of character\"}\\\],.*" \
>>>>> +    "\\^done,numchild=\"2\",children=\\\[child={name=\"vta.n\",exp=\"n\",numchild=\"0\",type=\"bar\\.int\",thread-id=\"1\"},child={name=\"vta.f\",exp=\"f\",numchild=\"0\",type=\"array \\(1 .. n\\) of character\",thread-id=\"1\"}\\\],.*" \
>>>>>      "list vta's children"
>>>>> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
>>>>> index 85f328d..bdd43e0 100644
>>>>> --- a/gdb/testsuite/gdb.mi/mi-break.exp
>>>>> +++ b/gdb/testsuite/gdb.mi/mi-break.exp
>>>>> @@ -213,7 +213,7 @@ proc test_error {} {
>>>>>      # containing function call, the internal breakpoint created to handle
>>>>>      # function call would be reported, messing up MI output.
>>>>>      mi_gdb_test "-var-create V * return_1()" \
>>>>> -        "\\^done,name=\"V\",numchild=\"0\",value=\"1\",type=\"int\",has_more=\"0\"" \
>>>>> +        "\\^done,name=\"V\",numchild=\"0\",value=\"1\",type=\"int\",thread-id=\"1\",has_more=\"0\"" \
>>>>>          "create varobj for function call"
>>>>>  
>>>>>      mi_gdb_test "-var-update *" \
>>>>> 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..45f81d6
>>>>> --- /dev/null
>>>>> +++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp
>>>>> @@ -0,0 +1,183 @@
>>>>> +# 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 varobj for pc in frame 0"
>>>>> +
>>>>> +	set nframes 4
>>>>> +	for {set i 1} {$i < $nframes} {incr 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/testsuite/gdb.mi/mi-var-cmd.exp b/gdb/testsuite/gdb.mi/mi-var-cmd.exp
>>>>> index 558cd6c..68e3cf8 100644
>>>>> --- a/gdb/testsuite/gdb.mi/mi-var-cmd.exp
>>>>> +++ b/gdb/testsuite/gdb.mi/mi-var-cmd.exp
>>>>> @@ -381,7 +381,7 @@ mi_gdb_test "-var-update *" \
>>>>>  	"assign same value to func (update)"
>>>>>  
>>>>>  mi_gdb_test "-var-create array_ptr * array_ptr" \
>>>>> -	"\\^done,name=\"array_ptr\",numchild=\"1\",value=\"$hex <array>\",type=\"int \\*\",has_more=\"0\"" \
>>>>> +	"\\^done,name=\"array_ptr\",numchild=\"1\",value=\"$hex <array>\",type=\"int \\*\",thread-id=\"1\",has_more=\"0\"" \
>>>>>  	"create global variable array_ptr"
>>>>>  
>>>>>  mi_gdb_test "-var-assign array_ptr array2" \
>>>>> @@ -608,7 +608,7 @@ mi_check_varobj_value F 7 "check F inside callee"
>>>>>  # A varobj we fail to read during -var-update should be considered
>>>>>  # out of scope.
>>>>>  mi_gdb_test "-var-create null_ptr * **0" \
>>>>> -    {\^done,name="null_ptr",numchild="0",value=".*",type="int",has_more="0"} \
>>>>> +    {\^done,name="null_ptr",numchild="0",value=".*",type="int",thread-id="1"has_more="0"} \
>>>>>      "create null_ptr"
>>>>>  
>>>>>  # Allow this to succeed, if address zero is readable, although it
>>>>> diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
>>>>> index 3bcb36c..a8cc76f 100644
>>>>> --- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
>>>>> +++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
>>>>> @@ -49,6 +49,6 @@ mi_gdb_test "-gdb-set print object on" ".*"
>>>>>  # We use a explicit cast to (void *) as that is the
>>>>>  # type that caused the bug this testcase is testing for.
>>>>>  mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
>>>>> -	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
>>>>> +	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",thread-id=\"1\",has_more=\"0\"" \
>>>>>  	    "-var-create sp1 * \$sp"
>>>>>  gdb_exit
>>>>> diff --git a/gdb/varobj.c b/gdb/varobj.c
>>>>> index 6f56cba..1e3f192 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_global_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;
>>>>>  }
>>>>> @@ -2268,7 +2270,9 @@ value_of_root_1 (struct varobj **var_handle)
>>>>>    back_to = make_cleanup_restore_current_thread ();
>>>>>  
>>>>>    /* Determine whether the variable is still around.  */
>>>>> -  if (var->root->valid_block == NULL || var->root->floating)
>>>>> +  if (var->root->valid_block == NULL
>>>>> +      || BLOCK_SUPERBLOCK (var->root->valid_block) == NULL
>>>>> +      || var->root->floating)
>>>>>      within_scope = 1;
>>>>>    else if (var->root->thread_id == 0)
>>>>>      {
>>>>>
>>>>
>>>
>>
> I've created PR mi/20395 for this issue and marked it with the Target
> Milestone 7.12.  Since it's an issue of returning incorrect data I'm
> proposing that it's a blocker.
> thanks
> --Don
>