From patchwork Tue Jan 2 15:31:46 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 25180 Received: (qmail 47279 invoked by alias); 2 Jan 2018 15:32:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 47221 invoked by uid 89); 2 Jan 2018 15:32:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=talked, essential, pc, sp2 X-HELO: mail-wr0-f175.google.com Received: from mail-wr0-f175.google.com (HELO mail-wr0-f175.google.com) (209.85.128.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 02 Jan 2018 15:31:58 +0000 Received: by mail-wr0-f175.google.com with SMTP id 36so15560831wrh.1 for ; Tue, 02 Jan 2018 07:31:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:in-reply-to:references; bh=jxPvSkoPM5/NNhOO3/eUo5w0EWJey2p48tNOZ6jWVLQ=; b=ICjr4UAJfrWYQDmUkn+1iIy9qtRdb5kvFZtrnfwjedNn+GG9HoMLJfkDJfAoWkFL5q sR0Nkzw3kmUhvX0vJLyuZ870EqX0P0hKhT6uIueRYXHhsXyPxK2BQA3ss1js5OlKkhvJ wesCPYvZFhvxkgMRfWCXXZEnIOK4rCYlZTXR7jAIdk65/IsP1erXtQQ/aL7Ea5C002ZG h4382ZkSjwbAw/D3HbMK96bNK3T2Ed4ZKIESvSxlwRd8w0EPp4jKHBKiT9YbYhgHoPwJ eDIBew+rgWpgfvpeDQcU8oDzdpbbLffJ63XL9ISXNaAPjL4uajPaWKUgDtVFptUt5WSt 79tw== X-Gm-Message-State: AKGB3mJ6twqLMM/AgRVOAhz6NT22l/3vcWjCVASI/B21RQ7rXDJ4XIli nA/DzqJnwtOjWggokF43HYDR6Bp+ X-Google-Smtp-Source: ACJfBovOA7yCqWT39CWSUg6ooL2GPdV9SZgL+V+Td7c7Btyjsr+WL7HG0rEUQKt87PuhmWsghWrKxg== X-Received: by 10.223.133.143 with SMTP id 15mr31034226wrt.266.1514907115441; Tue, 02 Jan 2018 07:31:55 -0800 (PST) Received: from localhost ([81.141.199.69]) by smtp.gmail.com with ESMTPSA id x88sm25897701wrb.4.2018.01.02.07.31.54 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 02 Jan 2018 07:31:54 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: donb@codesourcery.com, simark@simark.ca, palves@redhat.com, Andrew Burgess Subject: [PATCHv5 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up Date: Tue, 2 Jan 2018 15:31:46 +0000 Message-Id: <5a49daa3793d0912fce93af9ad2fed63892d9899.1514905848.git.andrew.burgess@embecosm.com> In-Reply-To: References: In-Reply-To: References: X-IsSubscribed: yes When revising this patch Simon talked about using DEF_ENUM_FLAGS_TYPE for the 'track_type', but that doesn't seem to work for enums nested within a class (as he pointed out). I'm happy to move the enum innermost_block_tracker::track_type out of the class, and make it global (with a more descriptive name I guess) if that's what people would prefer, then I could use DEF_ENUM_FLAGS_TYPE. Otherwise, I think all of the review comments have been addressed. Thanks, Andrew --- 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. A varobj is created as either fixed, the expression is evaluated within the context of a specific frame, or floating, the expression is evaluated within the current frame, whatever that may be. When a varobj is created by -var-create we set two fields of the varobj to track the context in which the varobj was created, these two fields are varobj->root->frame and var->root->valid_block. If a varobj is of type fixed, then, when we subsequently try to reevaluate the expression associated with the varobj we must determine if the original frame (and block) is still available, if it is not then the varobj can no longer be evaluated. The problem is that for register expressions varobj->root->valid_block is not set correctly. This block 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 then seems like it should be to just update the innermost block when parsing register expressions, however, that solution causes several test regressions. The problem is that in some cases we rely on the expression parsing code not updating the innermost block for registers, one example is when we parse the expression for a 'display' command. The display commands treats registers like floating varobjs, but symbols are treated like fixed varobjs. So 'display $reg_name' will always show the value of '$reg_name' even as the user moves from frame to frame, while 'display my_variable' will only show 'my_variable' while it is in the current frame and/or block, when the user moves to a new frame and/or block (even one with a different 'my_variable' in) then the display of 'my_variable' stops. For the case of 'display', without the option to force fixed or floating expressions, the current behaviour is probably the best choice. For the varobj system though, we can choose between floating and fixed, and we should try to make this work for registers. There's only one existing test case that needs to be updated, in that test a fixed varobj is created using a register, the MI output now include the thread-id in which the varobj should be evaluated, which I believe is correct behaviour. I also added a new floating test case into the same test script, however, right now this also includes the thread-id in the expected output, which I believe is an existing gdb bug, which I plan to fix next. Tested on x86_64 Linux native and native-gdbserver, no regressions. gdb/ChangeLog: PR mi/20395 * ada-exp.y (write_var_from_sym): Pass extra parameter when updating innermost block. * parse.c (innermost_block_tracker::update): Take extra type parameter, and check types match before updating innermost block. (write_dollar_variable): Update innermost block for registers. * parser-defs.h (enum innermost_block_tracker::track_type): New. (innermost_block_tracker::reset): Take type parameter. (innermost_block_tracker::update): Take type parameter, and pass type through as needed. (innermost_block_tracker::type): New member. (operator|): New function for innermost_block_tracker::track_type. * varobj.c (varobj_create): Pass type when reseting innermost block. gdb/testsuite/ChangeLog: * gdb.mi/basics.c: Add new global. * gdb.mi/mi-frame-regs.exp: New file. * gdb.mi/mi-var-create-rtti.exp: Update expected results, add new case. --- gdb/ChangeLog | 17 +++ gdb/ada-exp.y | 2 +- gdb/parse.c | 9 +- gdb/parser-defs.h | 24 +++- gdb/testsuite/ChangeLog | 8 ++ gdb/testsuite/gdb.mi/basics.c | 2 + gdb/testsuite/gdb.mi/mi-frame-regs.exp | 186 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.mi/mi-var-create-rtti.exp | 5 +- gdb/varobj.c | 3 +- 9 files changed, 248 insertions(+), 8 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y index fa7a4d5b4e8..7500e7e4438 100644 --- a/gdb/ada-exp.y +++ b/gdb/ada-exp.y @@ -757,7 +757,7 @@ write_var_from_sym (struct parser_state *par_state, struct symbol *sym) { if (symbol_read_needs_frame (sym)) - innermost_block.update (block); + innermost_block.update (block, innermost_block_tracker::for_symbols); write_exp_elt_opcode (par_state, OP_VAR_VALUE); write_exp_elt_block (par_state, block); diff --git a/gdb/parse.c b/gdb/parse.c index 042df51bf8a..0abfed13962 100644 --- a/gdb/parse.c +++ b/gdb/parse.c @@ -126,9 +126,12 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR, the currently stored block, or if no block is stored yet. */ void -innermost_block_tracker::update (const struct block *b) +innermost_block_tracker::update (const struct block *b, + enum innermost_block_tracker::track_type t) { - if (m_innermost_block == NULL || contained_in (b, m_innermost_block)) + if ((m_type & t) != 0 + && (m_innermost_block == NULL + || contained_in (b, m_innermost_block))) m_innermost_block = b; } @@ -693,6 +696,8 @@ handle_register: str.ptr++; write_exp_string (ps, str); write_exp_elt_opcode (ps, OP_REGISTER); + innermost_block.update (expression_context_block, + innermost_block_tracker::for_registers); return; } diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h index 5265b394bea..36741ad050b 100644 --- a/gdb/parser-defs.h +++ b/gdb/parser-defs.h @@ -81,20 +81,27 @@ extern CORE_ADDR expression_context_pc; class innermost_block_tracker { public: + enum track_type + { + for_symbols = 0x1, + for_registers = 0x2 + }; + innermost_block_tracker () : m_innermost_block (NULL) { /* Nothing. */ } - void reset () + void reset (enum track_type t = for_symbols) { + m_type = t; m_innermost_block = NULL; } - void update (const struct block *b); + void update (const struct block *b, track_type t); void update (const struct block_symbol &bs) { - update (bs.block); + update (bs.block, for_symbols); } const struct block *block () const @@ -103,9 +110,20 @@ public: } private: + enum track_type m_type; const struct block *m_innermost_block; }; +/* Allow the enum for block type to be bitwise-or together. */ + +inline enum innermost_block_tracker::track_type operator| + (enum innermost_block_tracker::track_type a, + enum innermost_block_tracker::track_type b) +{ + return static_cast + (static_cast (a) | static_cast (b)); +} + /* The innermost context required by the stack and register variables we've encountered so far. This should be cleared before parsing an expression, and queried once the parse is complete. */ diff --git a/gdb/testsuite/gdb.mi/basics.c b/gdb/testsuite/gdb.mi/basics.c index 08d9ccae178..327f33c6eca 100644 --- a/gdb/testsuite/gdb.mi/basics.c +++ b/gdb/testsuite/gdb.mi/basics.c @@ -20,6 +20,8 @@ * on function calls. Useful to test printing frames, stepping, etc. */ +unsigned long long global_zero = 0; + int callee4 (void) { int A=1; 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 00000000000..413fa5c3c92 --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp @@ -0,0 +1,186 @@ +# Copyright 2018 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$" { + unresolved "get address of breakpoint $bpnum" + return "" + } + timeout { + unresolved "get address of breakpoint $bpnum (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_with_prefix do_floating_varobj_test {} { + global srcfile + global hex + global expect_out + + gdb_exit + if {[mi_gdb_start]} then { + fail "couldn't start gdb" + return + } + + 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 in callee$i" + + # 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 { return } + + # Check that the addresses are the same. + gdb_assert [expr $bpaddr == $pcval] "\$pc equals address of breakpoint in callee$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_with_prefix do_fixed_varobj_test {} { + global srcfile + global hex + + gdb_exit + if {[mi_gdb_start]} then { + fail "couldn't start gdb" + return + } + + 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 in callee3" + + # At the breakpoint in callee3 there are 4 frames. + # + # Create some varobj based on $pc in all frames. When we single + # step we expect the varobj for frame 0 to change, while the + # varobj for all other frames should be unchanged. + # + # Track in FIRST_UNCHANGING_VARNUM the number of the first varobj + # that is not in frame 0, varobj with a lower number we expect to + # change, while this and later varobj should not change. + # + # Track the number of the next varobj to be created in VARNUM. + set first_unchanging_varnum 0 + set varnum 1 + + for {set i 0} {$i < 4} {incr i} { + + if { $i == 1 } then { set first_unchanging_varnum $varnum } + + mi_gdb_test "-var-create --thread 1 --frame $i - \* \$pc" \ + "\\^done,.*value=\"$hex.*" \ + "create varobj for \$pc in frame $i" + incr varnum + + mi_gdb_test "-var-create --thread 1 --frame $i - \* \"global_zero + \$pc\"" \ + "\\^done,.*value=\"$hex.*" \ + "create varobj for 'global_zero + \$pc' in frame $i" + incr varnum + } + + # 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 changed for + # varobj in frame 0. + for {set i 1} {$i < $first_unchanging_varnum} {incr i} { + mi_gdb_test "-var-update 1 var$i" \ + "\\^done,(changelist=\\\[\{name=\"var$i\"\[^\\\]\]+\\\])" \ + "varobj var$i has changed" + } + + # Check that -var-update reports that the values are unchanged for + # varobj in frames other than 0. + for {set i $first_unchanging_varnum} {$i < $varnum} {incr i} { + mi_gdb_test "-var-update 1 var$i" \ + "\\^done,(changelist=\\\[\\\])" \ + "varobj var$i has not changed" + } +} + +do_fixed_varobj_test +do_floating_varobj_test diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp index b61c495ab41..9ea5784bcad 100644 --- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp +++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp @@ -49,6 +49,9 @@ 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=\"$decimal\",has_more=\"0\"" \ "-var-create sp1 * \$sp" +mi_gdb_test "-var-create sp2 @ ((void*)\$sp)" \ + "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",thread-id=\"$decimal\",has_more=\"0\"" \ + "-var-create sp2 @ \$sp" gdb_exit diff --git a/gdb/varobj.c b/gdb/varobj.c index 20dd09bd585..586bf5fc53b 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -311,7 +311,8 @@ varobj_create (const char *objname, } p = expression; - innermost_block.reset (); + innermost_block.reset (innermost_block_tracker::for_symbols + | innermost_block_tracker::for_registers); /* Wrap the call to parse expression, so we can return a sensible error. */ TRY