From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 50404 invoked by alias); 5 Aug 2016 18:22:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 50392 invoked by uid 89); 5 Aug 2016 18:22:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Step, substantially, succession, sensible 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 05 Aug 2016 18:22:07 +0000 Received: from svr-orw-fem-06.mgc.mentorg.com ([147.34.97.120]) by relay1.mentorg.com with esmtp id 1bVjkz-0000WR-BC from Don_Breazeal@mentor.com ; Fri, 05 Aug 2016 11:22:05 -0700 Received: from [172.30.5.43] (147.34.91.1) by svr-orw-fem-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server (TLS) id 14.3.224.2; Fri, 5 Aug 2016 11:22:05 -0700 Subject: [PING] [PATCH v2] Fix -var-update for registers in frames 1 and up To: , References: <1465854882-42527-1-git-send-email-donb@codesourcery.com> <57680C62.5060807@codesourcery.com> <5783B1BE.7010206@codesourcery.com> <578FE7E6.5070507@codesourcery.com> <5790FF6C.9040003@codesourcery.com> <579B80A6.7090207@codesourcery.com> From: Don Breazeal Message-ID: <57A4D947.6010908@codesourcery.com> Date: Fri, 05 Aug 2016 18:22:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <579B80A6.7090207@codesourcery.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg00093.txt.bz2 Hi. This patch has been in the queue for quite a while. ping. Thanks! --Don >>>> >>>> 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 >>>>>> >>>>>> * 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 >>>>>> Andrew Burgess >>>>>> >>>>>> * 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=\" pck.gadatatype\",has_more=\"0\"" \ >>>>>> + "\\^done,name=\"ggg1\",numchild=\"1\",value=\"{...}\",type=\" 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 . >>>>>> + >>>>>> +# 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 \",type=\"int \\*\",has_more=\"0\"" \ >>>>>> + "\\^done,name=\"array_ptr\",numchild=\"1\",value=\"$hex \",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 >> >