gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up
authorAndrew Burgess <andrew.burgess@embecosm.com>
Wed, 18 Oct 2017 19:07:19 +0000 (20:07 +0100)
committerAndrew Burgess <andrew.burgess@embecosm.com>
Sun, 21 Jan 2018 15:46:51 +0000 (15:46 +0000)
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_type): New enum.
(innermost_block_tracker::innermost_block_tracker): Initialise
m_types member.
(innermost_block_tracker::reset): Take type parameter.
(innermost_block_tracker::update): Take type parameter, and pass
type through as needed.
(innermost_block_tracker::m_types): New member.
* 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
gdb/ada-exp.y
gdb/parse.c
gdb/parser-defs.h
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.mi/basics.c
gdb/testsuite/gdb.mi/mi-frame-regs.exp [new file with mode: 0644]
gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
gdb/varobj.c

index d6bf7e3a272a38d4a34a14dbeea2890ba45e2c33..9da1d3bcb8d1e35f5ccb955ff5dbb4bc34ad3ce7 100644 (file)
@@ -1,3 +1,21 @@
+2018-01-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       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_type): New enum.
+       (innermost_block_tracker::innermost_block_tracker): Initialise
+       m_types member.
+       (innermost_block_tracker::reset): Take type parameter.
+       (innermost_block_tracker::update): Take type parameter, and pass
+       type through as needed.
+       (innermost_block_tracker::m_types): New member.
+       * varobj.c (varobj_create): Pass type when reseting innermost
+       block.
+
 2018-01-21  Andrew Burgess  <andrew.burgess@embecosm.com>
 
        * ada-exp.y (write_var_from_sym): Switch to innermost_block API.
index 56113186b97fe5a9a29118959d292f438d9cf8f1..ac4c341bfe941a0d7d9188469cb8e626b1487ab4 100644 (file)
@@ -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_FOR_SYMBOLS);
 
   write_exp_elt_opcode (par_state, OP_VAR_VALUE);
   write_exp_elt_block (par_state, block);
index ca5eb0238692bf795c0fa78621ab575bd5935de4..e3f1306a175abd20e926ca9a34ef43e0e8f32b35 100644 (file)
@@ -124,9 +124,12 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
 /* Documented at it's declaration.  */
 
 void
-innermost_block_tracker::update (const struct block *b)
+innermost_block_tracker::update (const struct block *b,
+                                innermost_block_tracker_types t)
 {
-  if (m_innermost_block == NULL || contained_in (b, m_innermost_block))
+  if ((m_types & t) != 0
+      && (m_innermost_block == NULL
+         || contained_in (b, m_innermost_block)))
     m_innermost_block = b;
 }
 
@@ -691,6 +694,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_FOR_REGISTERS);
   return;
 }
 
index 01ac0cd363a1da92e720ec5c937909b8d2caad69..c67b8d5691b6c2abe7fba375a3e347be5622ba13 100644 (file)
@@ -75,6 +75,24 @@ extern const struct block *expression_context_block;
    then look up the macro definitions active at that point.  */
 extern CORE_ADDR expression_context_pc;
 
+/* While parsing expressions we need to track the innermost lexical block
+   that we encounter.  In some situations we need to track the innermost
+   block just for symbols, and in other situations we want to track the
+   innermost block for symbols and registers.  These flags are used by the
+   innermost block tracker to control which blocks we consider for the
+   innermost block.  These flags can be combined together as needed.  */
+
+enum innermost_block_tracker_type
+{
+  /* Track the innermost block for symbols within an expression.  */
+  INNERMOST_BLOCK_FOR_SYMBOLS = (1 << 0),
+
+  /* Track the innermost block for registers within an expression.  */
+  INNERMOST_BLOCK_FOR_REGISTERS = (1 << 1)
+};
+DEF_ENUM_FLAGS_TYPE (enum innermost_block_tracker_type,
+                    innermost_block_tracker_types);
+
 /* When parsing expressions we track the innermost block that was
    referenced.  */
 
@@ -82,24 +100,32 @@ class innermost_block_tracker
 {
 public:
   innermost_block_tracker ()
-    : m_innermost_block (NULL)
+    : m_types (INNERMOST_BLOCK_FOR_SYMBOLS),
+      m_innermost_block (NULL)
   { /* Nothing.  */ }
 
   /* Reset the currently stored innermost block.  Usually called before
-     parsing a new expression.  */
-  void reset ()
+     parsing a new expression.  As the most common case is that we only
+     want to gather the innermost block for symbols in an expression, this
+     becomes the default block tracker type.  */
+  void reset (innermost_block_tracker_types t = INNERMOST_BLOCK_FOR_SYMBOLS)
   {
-    m_innermost_block = nullptr;
+    m_types = t;
+    m_innermost_block = NULL;
   }
 
   /* Update the stored innermost block if the new block B is more inner
-     than the currently stored block, or if no block is stored yet.  */
-  void update (const struct block *b);
+     than the currently stored block, or if no block is stored yet.  The
+     type T tells us whether the block B was for a symbol or for a
+     register.  The stored innermost block is only updated if the type T is
+     a type we are interested in, the types we are interested in are held
+     in M_TYPES and set during RESET.  */
+  void update (const struct block *b, innermost_block_tracker_types t);
 
   /* Overload of main UPDATE method which extracts the block from BS.  */
   void update (const struct block_symbol &bs)
   {
-    update (bs.block);
+    update (bs.block, INNERMOST_BLOCK_FOR_SYMBOLS);
   }
 
   /* Return the stored innermost block.  Can be nullptr if no symbols or
@@ -111,13 +137,16 @@ public:
   }
 
 private:
+  /* The type of innermost block being looked for.  */
+  innermost_block_tracker_types m_types;
+
   /* The currently stored innermost block found while parsing an
      expression.  */
   const struct block *m_innermost_block;
 };
 
-/* The innermost context required by the stack and register variables we've
-   encountered so far.  This should be cleared before parsing an
+/* 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.  */
 extern innermost_block_tracker innermost_block;
 
index db860b0a1f831f569d54866ca87c1ca992c9af5e..3a8e6ed30d2c5960082375cea7b23fb2997c1bc1 100644 (file)
@@ -1,3 +1,11 @@
+2018-01-21  Don Breazeal  <donb@codesourcery.com>
+           Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * 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.
+
 2018-01-21  Andrew Burgess  <andrew.burgess@embecosm.com>
 
        * gdb.arch/amd64-entry-value.exp: Test using @entry on a
index 08d9ccae178f3c2215fc01b94ca186e3907c5f15..327f33c6eca1c1ad07f4593f7bef72dee558fabf 100644 (file)
@@ -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 (file)
index 0000000..413fa5c
--- /dev/null
@@ -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 <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$" {
+           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
index b61c495ab41c217d4c21f516149795520dce1e9c..9ea5784bcadb16cbaab231ce625e764a483f452f 100644 (file)
@@ -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
index 20dd09bd5858c8a64ee3158a72aa1b02ca0ce2c9..6c0145402ab2b492cfd757dabdd1c7a4418c623f 100644 (file)
@@ -311,7 +311,8 @@ varobj_create (const char *objname,
        }
 
       p = expression;
-      innermost_block.reset ();
+      innermost_block.reset (INNERMOST_BLOCK_FOR_SYMBOLS
+                            | INNERMOST_BLOCK_FOR_REGISTERS);
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY