[PATCHv4 4/5] gdb: Remove out of date comment

[PATCHv4 4/5] gdb: Remove out of date comment

[Andrew Burgess]

Comment clean up.

gdb/ChangeLog:

	* varobj.c (varobj_create): Remove out of date comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/varobj.c  | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 56affc73f0..d8b927ef4f 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -311,7 +311,6 @@ varobj_create (const char *objname,
       else
 	fi = NULL;
 
-      /* frame = -2 means always use selected frame.  */
       if (type == USE_SELECTED_FRAME)
 	var->root->floating = 1;
 
-- 
2.12.2

[PATCHv4 5/5] gdb: Don't store a thread-id for floating varobj

[Andrew Burgess]

When creating a varobj with -var-create a user can create either fixed
varobj, or floating varobj.

A fixed varobj will always be evaluated within the thread/frame/block in
which the varobj was created, if that thread/frame/block is no longer
available then the varobj is considered out of scope.

A floating varobj will always be evaluated within the current
thread/frame/block.

Despite never using them GDB was storing the thread/frame/block into a
floating varobj, and the thread-id would then be displayed when GDB
reported on the state of the varobj, this could confuse a user into
thinking that the thread-id was relevant.

This commit prevents GDB storing the thread/frame/block onto floating
varobj, and updates the few tests where this impacts the results.

gdb/ChangeLog:

	* varobj.c (varobj_create): Don't set valid_block when creating a
	floating varobj.

gdb/testsuite/ChangeLog:

	* gdb.python/py-mi.exp: Don't expect a thread-id for floating
	varobj.
	* gdb.mi/mi-var-create-rtti.exp: Likewise.
---
 gdb/ChangeLog                               |  5 +++++
 gdb/testsuite/ChangeLog                     |  6 ++++++
 gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |  2 +-
 gdb/testsuite/gdb.python/py-mi.exp          | 12 ++++++------
 gdb/varobj.c                                |  3 ++-
 5 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index afc9c248ae..85ead72e7b 100644
--- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
@@ -52,6 +52,6 @@ mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
 	    "\\^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\"" \
+	    "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
 	    "-var-create sp2 @ \$sp"
 gdb_exit
diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index 736dc7a0d6..aa9a2f12a2 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -101,7 +101,7 @@ mi_varobj_update_dynamic container "varobj update 1" {
     type_changed false new_num_children 1 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
 }
 
 mi_next "next over update 2"
@@ -110,7 +110,7 @@ mi_varobj_update_dynamic container "varobj update 2" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container None" \
@@ -129,8 +129,8 @@ mi_varobj_update_dynamic container "varobj update after choosing default" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container ContainerPrinter" \
@@ -142,8 +142,8 @@ mi_varobj_update_dynamic container \
       type_changed false new_num_children 2 dynamic 1 has_more 0
   } {
   } {
-      { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-      { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+      { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+      { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
   }
 
 mi_list_varobj_children_range container 1 2 2 {
diff --git a/gdb/varobj.c b/gdb/varobj.c
index d8b927ef4f..ee555b39ac 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -351,7 +351,8 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var);
-      var->root->valid_block = innermost_block.block ();
+      var->root->valid_block =
+	var->root->floating ? NULL : innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
-- 
2.12.2

[PATCHv4 1/5] gdb: Remove duplicate declaration of a function

[Andrew Burgess]

A function is declared in two header files.  Remove the extra
declaration, and add an include of the correct header into the one place
this seems cause an issue.

gdb/ChangeLog:

	* expression.h (innermost_block): Remove declaration.
	* varobj.c: Add 'parser-defs.h' include.
---
 gdb/ChangeLog    | 5 +++++
 gdb/expression.h | 5 -----
 gdb/varobj.c     | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/expression.h b/gdb/expression.h
index 9e4ddf5ded..396ddf776a 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -113,11 +113,6 @@ extern expression_up parse_exp_1 (const char **, CORE_ADDR pc,
    attempt completion.  */
 extern int parse_completion;
 
-/* The innermost context required by the stack and register variables
-   we've encountered so far.  To use this, set it to NULL, then call
-   parse_<whatever>, then look at it.  */
-extern const struct block *innermost_block;
-
 /* From eval.c */
 
 /* Values of NOSIDE argument to eval_subexp.  */
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 2d850fb5e1..f2232888c4 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -30,6 +30,7 @@
 #include "gdbthread.h"
 #include "inferior.h"
 #include "varobj-iter.h"
+#include "parser-defs.h"
 
 #if HAVE_PYTHON
 #include "python/python.h"
-- 
2.12.2

[PATCHv4 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

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 got 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      | 187 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |   5 +-
 gdb/varobj.c                                |   3 +-
 9 files changed, 249 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 ff5650672c..6a5548093b 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -761,7 +761,7 @@ write_var_from_sym (struct parser_state *par_state,
 		    struct symbol *sym)
 {
   if (orig_left_context == NULL && 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 945ef295e6..0333623ee8 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 (innermost_block == NULL || contained_in (b, innermost_block))
+  if ((type & t) != 0
+      && (innermost_block == NULL
+	  || contained_in (b, innermost_block)))
     innermost_block = b;
 }
 
@@ -707,6 +710,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 d9de10dda8..aaf9ce27bd 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -69,20 +69,27 @@ extern CORE_ADDR expression_context_pc;
 class innermost_block_tracker
 {
 public:
+  enum track_type
+    {
+     for_symbols = 0x1,
+     for_registers = 0x2
+    };
+
   innermost_block_tracker ()
     : innermost_block (NULL)
   { /* Nothing.  */ }
 
-  void reset ()
+  void reset (enum track_type t = for_symbols)
   {
+    type = t;
     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
@@ -91,9 +98,20 @@ public:
   }
 
 private:
+  enum track_type type;
   const struct block *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<enum innermost_block_tracker::track_type>
+    (static_cast<int> (a) | static_cast<int> (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 9105e90d1b..d7f024766e 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 0000000000..7a0fff26ac
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp
@@ -0,0 +1,187 @@
+# 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"
+
+	mi_gdb_test "-var-create --thread 1 --frame $i - \* \"global_zero + \$pc\"" \
+		    "\\^done,.*value=\"$hex.*" \
+	            "create varobj for 'global_zero + 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-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index ffd448656f..afc9c248ae 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 5b5ffd2baf..56affc73f0 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -324,7 +324,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
-- 
2.12.2

[PATCHv4 0/5] Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

This patch revives a patch series that was first discussed here:

  https://sourceware.org/ml/gdb-patches/2016-06/msg00145.html

with v3 being posted here:

  https://sourceware.org/ml/gdb-patches/2016-10/msg00069.html

On reflection I worried that the approach taken in patch v3 might not
cover all the cases, specifically, it would only spot cases where the
register was the first operand in the expression.  So something like
this would not get the correct results:

  (gdb) -var-create - * "(void*) $sp"

The more I thought about it, the more I wanted to revisit this
suggestion:

  https://sourceware.org/ml/gdb-patches/2016-06/msg00159.html

In this patch I extend the expression parser to log the innermost
block when parsing a register expression, however, Don pointed out at
the time, that this causes some test failures, as in some cases we
don't want to log the innermost block for registers.

The solution I'm proposing is to make the expression parser smarter,
or rather, the innermost block tracking aspect of the expression
parser.

Previously the innermost block was a simple global pointer, which we
set to NULL before parsing an expression, and read out afterwards.
Now, the innermost block is an object, we reset the object before
parsing an expression, and tell the object which types of innermost
block we care about.  During expression parsing all blocks are
mentioned to the innermost block object, and it takes care of
remembering the correct, innermost block.  After the expression parse
we query the innermost block object to find the correct answer.

There's also a very related fix thrown in at the end of the series, in
patch #5.

There are new tests added, and results updated in the few places where
things need to change, which is mainly as a result of the fix in patch #5.

Tested on x86-64 Linux native and with native-gdbserver, no regressions.

Thanks,
Andrew

---

Andrew Burgess (5):
  gdb: Remove duplicate declaration of a function
  gdb: New API for tracking innermost block
  gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up
  gdb: Remove out of date comment
  gdb: Don't store a thread-id for floating varobj

 gdb/ChangeLog                               |  54 ++++++++
 gdb/ada-exp.y                               |   6 +-
 gdb/ada-lang.c                              |   8 +-
 gdb/breakpoint.c                            |  12 +-
 gdb/c-exp.y                                 |  20 +--
 gdb/d-exp.y                                 |  11 +-
 gdb/expression.h                            |   5 -
 gdb/f-exp.y                                 |   7 +-
 gdb/go-exp.y                                |   7 +-
 gdb/m2-exp.y                                |  14 +--
 gdb/objfiles.c                              |   2 +-
 gdb/p-exp.y                                 |  12 +-
 gdb/parse.c                                 |  18 ++-
 gdb/parser-defs.h                           |  54 +++++++-
 gdb/printcmd.c                              |   8 +-
 gdb/rust-exp.y                              |   8 +-
 gdb/symfile.c                               |   2 +-
 gdb/testsuite/ChangeLog                     |  14 +++
 gdb/testsuite/gdb.mi/basics.c               |   2 +
 gdb/testsuite/gdb.mi/mi-frame-regs.exp      | 187 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |   5 +-
 gdb/testsuite/gdb.python/py-mi.exp          |  12 +-
 gdb/varobj.c                                |  10 +-
 23 files changed, 372 insertions(+), 106 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp

-- 
2.12.2

[PATCHv4 2/5] gdb: New API for tracking innermost block

[Andrew Burgess]

This commit is preparation for a later change, at this point there
should be no user visible change.

We currently maintain a global innermost_block which tracks the most
inner block encountered when parsing an expression.

This commit wraps the innermost_block into a new class, and switches all
direct accesses to the variable to use the class API.

gdb/ChangeLog:

	* ada-exp.y (write_var_from_sym): Switch to innermost_block API.
	* ada-lang.c (resolve_subexp): Likewise.
	* breakpoint.c (set_breakpoint_condition) Likewise.
	(watch_command_1) Likewise.
	* c-exp.y (variable): Likewise.
	* d-exp.y (PrimaryExpression): Likewise.
	* f-exp.y (variable): Likewise.
	* go-exp.y (variable): Likewise.
	* m2-exp.y (variable): Likewise.
	* objfiles.c (objfile::~objfile): Likewise.
	* p-exp.y (variable): Likewise.
	* parse.c (innermost_block): Change type.
	* parser-defs.h (class innermost_block_tracker): New.
	(innermost_block): Change to innermost_block_tracker.
	* printcmd.c (display_command): Switch to innermost_block API.
	(do_one_display): Likewise.
	* rust-exp.y (do_one_display): Likewise.
	* symfile.c (clear_symtab_users): Likewise.
	* varobj.c (varobj_create): Switch to innermost_block API, replace
	use of innermost_block with block stored on varobj object.
---
 gdb/ChangeLog     | 23 +++++++++++++++++++++++
 gdb/ada-exp.y     |  6 +-----
 gdb/ada-lang.c    |  8 ++------
 gdb/breakpoint.c  | 12 ++++++------
 gdb/c-exp.y       | 20 ++++----------------
 gdb/d-exp.y       | 11 ++---------
 gdb/f-exp.y       |  7 +------
 gdb/go-exp.y      |  7 +------
 gdb/m2-exp.y      | 14 ++------------
 gdb/objfiles.c    |  2 +-
 gdb/p-exp.y       | 12 ++----------
 gdb/parse.c       | 13 ++++++++++++-
 gdb/parser-defs.h | 36 ++++++++++++++++++++++++++++++++++--
 gdb/printcmd.c    |  8 ++++----
 gdb/rust-exp.y    |  8 +++-----
 gdb/symfile.c     |  2 +-
 gdb/varobj.c      |  6 +++---
 17 files changed, 102 insertions(+), 93 deletions(-)

diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 4c1ff7b493..ff5650672c 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -761,11 +761,7 @@ write_var_from_sym (struct parser_state *par_state,
 		    struct symbol *sym)
 {
   if (orig_left_context == NULL && symbol_read_needs_frame (sym))
-    {
-      if (innermost_block == 0
-	  || contained_in (block, innermost_block))
-	innermost_block = block;
-    }
+    innermost_block.update (block);
 
   write_exp_elt_opcode (par_state, OP_VAR_VALUE);
   write_exp_elt_block (par_state, block);
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7409496ce7..2b47e7ee6d 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3509,9 +3509,7 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
 
           exp->elts[pc + 1].block = candidates[i].block;
           exp->elts[pc + 2].symbol = candidates[i].symbol;
-          if (innermost_block == NULL
-              || contained_in (candidates[i].block, innermost_block))
-            innermost_block = candidates[i].block;
+	  innermost_block.update (candidates[i]);
         }
 
       if (deprocedure_p
@@ -3554,9 +3552,7 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
 
             exp->elts[pc + 4].block = candidates[i].block;
             exp->elts[pc + 5].symbol = candidates[i].symbol;
-            if (innermost_block == NULL
-                || contained_in (candidates[i].block, innermost_block))
-              innermost_block = candidates[i].block;
+	    innermost_block.update (candidates[i]);
           }
       }
       break;
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 32ceea7c9b..665ca3e76d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -905,12 +905,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  arg = exp;
 	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
 	  if (*arg)
 	    error (_("Junk at end of expression"));
-	  w->cond_exp_valid_block = innermost_block;
+	  w->cond_exp_valid_block = innermost_block.block ();
 	}
       else
 	{
@@ -10777,7 +10777,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   /* Parse the rest of the arguments.  From here on out, everything
      is in terms of a newly allocated string instead of the original
      ARG.  */
-  innermost_block = NULL;
+  innermost_block.reset ();
   std::string expression (arg, exp_end - arg);
   exp_start = arg = expression.c_str ();
   expression_up exp = parse_exp_1 (&arg, 0, 0, 0);
@@ -10799,7 +10799,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       error (_("Cannot watch constant value `%.*s'."), len, exp_start);
     }
 
-  exp_valid_block = innermost_block;
+  exp_valid_block = innermost_block.block ();
   mark = value_mark ();
   fetch_subexp_value (exp.get (), &pc, &val, &result, NULL, just_location);
 
@@ -10837,13 +10837,13 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   toklen = end_tok - tok;
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
-      innermost_block = NULL;
+      innermost_block.reset ();
       tok = cond_start = end_tok + 1;
       parse_exp_1 (&tok, 0, 0, 0);
 
       /* The watchpoint expression may not be local, but the condition
 	 may still be.  E.g.: `watch global if local > 0'.  */
-      cond_exp_valid_block = innermost_block;
+      cond_exp_valid_block = innermost_block.block ();
 
       cond_end = tok;
     }
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 18c74d8a17..c5df14f252 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -962,12 +962,8 @@ variable:	block COLONCOLON name
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -1056,12 +1052,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -1073,10 +1064,7 @@ variable:	name_not_typename
 			      /* C++: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index 00c96764d6..c1f11ad1fa 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -422,12 +422,7 @@ PrimaryExpression:
 		  if (sym.symbol && SYMBOL_CLASS (sym.symbol) != LOC_TYPEDEF)
 		    {
 		      if (symbol_read_needs_frame (sym.symbol))
-			{
-			  if (innermost_block == 0
-			      || contained_in (sym.block, innermost_block))
-			    innermost_block = sym.block;
-			}
-
+			innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 		      write_exp_elt_block (pstate, sym.block);
 		      write_exp_elt_sym (pstate, sym.symbol);
@@ -437,9 +432,7 @@ PrimaryExpression:
 		     {
 		      /* It hangs off of `this'.  Must not inadvertently convert from a
 			 method call to data ref.  */
-		      if (innermost_block == 0
-			  || contained_in (sym.block, innermost_block))
-			innermost_block = sym.block;
+		      innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index 8dcc811b68..6e01e8d442 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -460,12 +460,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
 			      write_exp_elt_sym (pstate, sym.symbol);
diff --git a/gdb/go-exp.y b/gdb/go-exp.y
index 629093a118..5d808fe9dc 100644
--- a/gdb/go-exp.y
+++ b/gdb/go-exp.y
@@ -554,12 +554,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 02dc36f480..5fc43d08f5 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -548,12 +548,7 @@ variable:	block COLONCOLON NAME
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -574,12 +569,7 @@ variable:	NAME
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0 ||
-				      contained_in (sym.block,
-						    innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index d8fe88b136..0a6398e54d 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -704,7 +704,7 @@ objfile::~objfile ()
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Check to see if the current_source_symtab belongs to this objfile,
      and if so, call clear_current_source_symtab_and_line.  */
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index eee4fa94b3..68433a90b4 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -709,12 +709,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -728,10 +723,7 @@ variable:	name_not_typename
 			      /* Object pascal: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/parse.c b/gdb/parse.c
index 6bbf25f699..945ef295e6 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -68,7 +68,7 @@ const struct exp_descriptor exp_descriptor_standard =
 /* Global variables declared in parser-defs.h (and commented there).  */
 const struct block *expression_context_block;
 CORE_ADDR expression_context_pc;
-const struct block *innermost_block;
+innermost_block_tracker innermost_block;
 int arglist_len;
 static struct type_stack type_stack;
 const char *lexptr;
@@ -121,6 +121,17 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
 					     const struct block *, int,
 					     int, int *);
 
+
+/* Update the stored innermost block if the new block is more inner than
+   the currently stored block, or if no block is stored yet.  */
+
+void
+innermost_block_tracker::update (const struct block *b)
+{
+  if (innermost_block == NULL || contained_in (b, innermost_block))
+    innermost_block = b;
+}
+
 /* Data structure for saving values of arglist_len for function calls whose
    arguments contain other function calls.  */
 
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index f7ba7f088c..d9de10dda8 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -63,9 +63,41 @@ extern const struct block *expression_context_block;
    then look up the macro definitions active at that point.  */
 extern CORE_ADDR expression_context_pc;
 
+/* When parsing expressions we track the innermost block that was
+   referenced.  These functions are described in more detail at their
+   definition site.  */
+class innermost_block_tracker
+{
+public:
+  innermost_block_tracker ()
+    : innermost_block (NULL)
+  { /* Nothing.  */ }
+
+  void reset ()
+  {
+    innermost_block = NULL;
+  }
+
+  void update (const struct block *b);
+
+  void update (const struct block_symbol &bs)
+  {
+    update (bs.block);
+  }
+
+  const struct block *block () const
+  {
+    return innermost_block;
+  }
+
+private:
+  const struct block *innermost_block;
+};
+
 /* The innermost context required by the stack and register variables
-   we've encountered so far.  */
-extern const struct block *innermost_block;
+   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;
 
 /* Number of arguments seen so far in innermost function call.  */
 extern int arglist_len;
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 77a05e5d9f..c443b5b945 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1721,14 +1721,14 @@ display_command (char *arg, int from_tty)
       fmt.raw = 0;
     }
 
-  innermost_block = NULL;
+  innermost_block.reset ();
   expression_up expr = parse_expression (exp);
 
   newobj = new display ();
 
   newobj->exp_string = xstrdup (exp);
   newobj->exp = std::move (expr);
-  newobj->block = innermost_block;
+  newobj->block = innermost_block.block ();
   newobj->pspace = current_program_space;
   newobj->number = ++display_number;
   newobj->format = fmt;
@@ -1889,9 +1889,9 @@ do_one_display (struct display *d)
 
       TRY
 	{
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  d->exp = parse_expression (d->exp_string);
-	  d->block = innermost_block;
+	  d->block = innermost_block.block ();
 	}
       CATCH (ex, RETURN_MASK_ALL)
 	{
diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y
index 0cb185c512..403c5cbfdb 100644
--- a/gdb/rust-exp.y
+++ b/gdb/rust-exp.y
@@ -1049,15 +1049,13 @@ super_name (const struct rust_op *ident, unsigned int n_supers)
 		   ident->right.params);
 }
 
-/* A helper that updates innermost_block as appropriate.  */
+/* A helper that updates the innermost block as appropriate.  */
 
 static void
 update_innermost_block (struct block_symbol sym)
 {
-  if (symbol_read_needs_frame (sym.symbol)
-      && (innermost_block == NULL
-	  || contained_in (sym.block, innermost_block)))
-    innermost_block = sym.block;
+  if (symbol_read_needs_frame (sym.symbol))
+    innermost_block.update (sym);
 }
 
 /* A helper to look up a Rust type, or fail.  This only works for
diff --git a/gdb/symfile.c b/gdb/symfile.c
index a7d8553bb0..40c087fa4d 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2890,7 +2890,7 @@ clear_symtab_users (symfile_add_flags add_flags)
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Varobj may refer to old symbols, perform a cleanup.  */
   varobj_invalidate ();
diff --git a/gdb/varobj.c b/gdb/varobj.c
index f2232888c4..5b5ffd2baf 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -324,7 +324,7 @@ varobj_create (const char *objname,
 	}
 
       p = expression;
-      innermost_block = NULL;
+      innermost_block.reset ();
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
@@ -351,7 +351,7 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var);
-      var->root->valid_block = innermost_block;
+      var->root->valid_block = innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
@@ -360,7 +360,7 @@ varobj_create (const char *objname,
          we must select the appropriate frame before parsing
          the expression, otherwise the value will not be current.
          Since select_frame is so benign, just call it for all cases.  */
-      if (innermost_block)
+      if (var->root->valid_block)
 	{
 	  /* User could specify explicit FRAME-ADDR which was not found but
 	     EXPRESSION is frame specific and we would not be able to evaluate
-- 
2.12.2

Re: [PATCHv4 1/5] gdb: Remove duplicate declaration of a function

[Simon Marchi]

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> A function is declared in two header files.  Remove the extra

function, or global variable?

> declaration, and add an include of the correct header into the one place
> this seems cause an issue.

What issue?

The change in itself looks good to me.

Simon

Re: [PATCHv4 2/5] gdb: New API for tracking innermost block

[Simon Marchi]

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> This commit is preparation for a later change, at this point there
> should be no user visible change.
> 
> We currently maintain a global innermost_block which tracks the most
> inner block encountered when parsing an expression.
> 
> This commit wraps the innermost_block into a new class, and switches all
> direct accesses to the variable to use the class API.

Hi Andrew,

I think this is a nice cleanup on its own right, removing a lot of duplicated
code.  LGTM with a nit:

> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index f7ba7f088c..d9de10dda8 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -63,9 +63,41 @@ extern const struct block *expression_context_block;
>     then look up the macro definitions active at that point.  */
>  extern CORE_ADDR expression_context_pc;
>  
> +/* When parsing expressions we track the innermost block that was
> +   referenced.  These functions are described in more detail at their
> +   definition site.  */
> +class innermost_block_tracker
> +{
> +public:
> +  innermost_block_tracker ()
> +    : innermost_block (NULL)
> +  { /* Nothing.  */ }
> +
> +  void reset ()
> +  {
> +    innermost_block = NULL;
> +  }
> +
> +  void update (const struct block *b);
> +
> +  void update (const struct block_symbol &bs)
> +  {
> +    update (bs.block);
> +  }
> +
> +  const struct block *block () const
> +  {
> +    return innermost_block;
> +  }
> +
> +private:
> +  const struct block *innermost_block;

Private members should be prefixed with m_ (m_innermost_block).

Simon

Re: [PATCHv4 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Simon Marchi]

Hi Andrew,

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> 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 got registers.

The "got" seems misplaced here, what did you mean to say?

> 
> 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.

I read the (quite long) history of this patch series to catch up, and
the approach seems good to me.  I think you have addressed Pedro's
concerns (from 2016-08), but I'll let him take a look, if he has time.

I noted a few minor comments, mostly in the new test.

> gdb/ChangeLog:
> 
>         PR mi/20395

Make sure to use a tab here instead of spaces.

> 	* 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      | 187 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |   5 +-
>  gdb/varobj.c                                |   3 +-
>  9 files changed, 249 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 ff5650672c..6a5548093b 100644
> --- a/gdb/ada-exp.y
> +++ b/gdb/ada-exp.y
> @@ -761,7 +761,7 @@ write_var_from_sym (struct parser_state *par_state,
>  		    struct symbol *sym)
>  {
>    if (orig_left_context == NULL && 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 945ef295e6..0333623ee8 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 (innermost_block == NULL || contained_in (b, innermost_block))
> +  if ((type & t) != 0
> +      && (innermost_block == NULL
> +	  || contained_in (b, innermost_block)))
>      innermost_block = b;
>  }
>  
> @@ -707,6 +710,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 d9de10dda8..aaf9ce27bd 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -69,20 +69,27 @@ extern CORE_ADDR expression_context_pc;
>  class innermost_block_tracker
>  {
>  public:
> +  enum track_type
> +    {
> +     for_symbols = 0x1,
> +     for_registers = 0x2
> +    };
> +
>    innermost_block_tracker ()
>      : innermost_block (NULL)
>    { /* Nothing.  */ }
>  
> -  void reset ()
> +  void reset (enum track_type t = for_symbols)
>    {
> +    type = t;
>      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
> @@ -91,9 +98,20 @@ public:
>    }
>  
>  private:
> +  enum track_type type;
>    const struct block *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<enum innermost_block_tracker::track_type>
> +    (static_cast<int> (a) | static_cast<int> (b));
> +}

track_type would ideally be an enum_flags type.  Unfortunately, it doesn't
seem to work to put DEF_ENUM_FLAGS_TYPE in a class, so I assume this is why
you've done it this way.  I'm fine with this, we can look into improving
enum flags later (if it's even possible).  Otherwise, I'd be fine with putting
the type outside the class.

>  /* 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 9105e90d1b..d7f024766e 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 0000000000..7a0fff26ac
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp
> @@ -0,0 +1,187 @@
> +# Copyright 1999-2016 Free Software Foundation, Inc.

This should probably say -2017.

> +
> +# 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 ""

Should we put a FAIL here?  If this happens, it means the test isn't going as planned.

> +	}
> +	timeout {
> +	    return ""

Same here.

I just saw in the caller you check if the return value is "".  I think it would still
be good to put a FAIL (or UNRESOLVED) here, in case somebody uses this function without
checking the return value.  And if they do:

  set addr [breakpoint_address 1]  # this returns ""
  gdb_test "some_command" ".*$addr.*"

the failure would go unnoticced.

> +	}
> +    }
> +}
> +
> +# 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

This "continue" is not in a loop, so it wouldn't work.  I suggest

  fail "couldn't start gdb"
  return

> +    }
> +
> +    with_test_prefix "floating" {

Instead of with_test_prefix, I would suggest declaring the proc with "proc_with_prefix".
The name of the proc is expressive enough to be used as the "scope".

> +	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"
> +	    }

I think it would be good to use the same test name if possible.  It's not a huge deal, but
in the buildbot for example, if this test starts failing, it would appear as a new test
that fails, rather than a PASS -> FAIL.

For that purpose, you can use gdb_assert:

  gdb_assert "$bpaddr == $pcval" "\$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"
> +
> +	mi_gdb_test "-var-create --thread 1 --frame $i - \* \"global_zero + \$pc\"" \
> +		    "\\^done,.*value=\"$hex.*" \
> +	            "create varobj for 'global_zero + 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} {
> +

While at it, would it be good to create a varobj frame 0's $pc and confirm
that it did change?

> +    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

I don't think these last two lines are needed.


> diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> index ffd448656f..afc9c248ae 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 5b5ffd2baf..56affc73f0 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -324,7 +324,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
> 

Thanks,

Simon

Re: [PATCHv4 4/5] gdb: Remove out of date comment

[Simon Marchi]

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> Comment clean up.
> 
> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_create): Remove out of date comment.
> ---
>  gdb/ChangeLog | 4 ++++
>  gdb/varobj.c  | 1 -
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 56affc73f0..d8b927ef4f 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -311,7 +311,6 @@ varobj_create (const char *objname,
>        else
>  	fi = NULL;
>  
> -      /* frame = -2 means always use selected frame.  */
>        if (type == USE_SELECTED_FRAME)
>  	var->root->floating = 1;
>  
> 

LGTM.

Re: [PATCHv4 5/5] gdb: Don't store a thread-id for floating varobj

[Simon Marchi]

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> When creating a varobj with -var-create a user can create either fixed
> varobj, or floating varobj.
> 
> A fixed varobj will always be evaluated within the thread/frame/block in
> which the varobj was created, if that thread/frame/block is no longer
> available then the varobj is considered out of scope.
> 
> A floating varobj will always be evaluated within the current
> thread/frame/block.
> 
> Despite never using them GDB was storing the thread/frame/block into a
> floating varobj, and the thread-id would then be displayed when GDB
> reported on the state of the varobj, this could confuse a user into
> thinking that the thread-id was relevant.
> 
> This commit prevents GDB storing the thread/frame/block onto floating
> varobj, and updates the few tests where this impacts the results.
> 
> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_create): Don't set valid_block when creating a
> 	floating varobj.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-mi.exp: Don't expect a thread-id for floating
> 	varobj.
> 	* gdb.mi/mi-var-create-rtti.exp: Likewise.
> ---
>  gdb/ChangeLog                               |  5 +++++
>  gdb/testsuite/ChangeLog                     |  6 ++++++
>  gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |  2 +-
>  gdb/testsuite/gdb.python/py-mi.exp          | 12 ++++++------
>  gdb/varobj.c                                |  3 ++-
>  5 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> index afc9c248ae..85ead72e7b 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> @@ -52,6 +52,6 @@ mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
>  	    "\\^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\"" \
> +	    "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
>  	    "-var-create sp2 @ \$sp"
>  gdb_exit
> diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
> index 736dc7a0d6..aa9a2f12a2 100644
> --- a/gdb/testsuite/gdb.python/py-mi.exp
> +++ b/gdb/testsuite/gdb.python/py-mi.exp
> @@ -101,7 +101,7 @@ mi_varobj_update_dynamic container "varobj update 1" {
>      type_changed false new_num_children 1 dynamic 1 has_more 0
>  } {
>  } {
> -    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
> +    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
>  }
>  
>  mi_next "next over update 2"
> @@ -110,7 +110,7 @@ mi_varobj_update_dynamic container "varobj update 2" {
>      type_changed false new_num_children 2 dynamic 1 has_more 0
>  } {
>  } {
> -    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
> +    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
>  }
>  
>  mi_gdb_test "-var-set-visualizer container None" \
> @@ -129,8 +129,8 @@ mi_varobj_update_dynamic container "varobj update after choosing default" {
>      type_changed false new_num_children 2 dynamic 1 has_more 0
>  } {
>  } {
> -    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
> -    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
> +    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
> +    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
>  }
>  
>  mi_gdb_test "-var-set-visualizer container ContainerPrinter" \
> @@ -142,8 +142,8 @@ mi_varobj_update_dynamic container \
>        type_changed false new_num_children 2 dynamic 1 has_more 0
>    } {
>    } {
> -      { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
> -      { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
> +      { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
> +      { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
>    }
>  
>  mi_list_varobj_children_range container 1 2 2 {
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index d8b927ef4f..ee555b39ac 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -351,7 +351,8 @@ varobj_create (const char *objname,
>  	}
>  
>        var->format = variable_default_display (var);
> -      var->root->valid_block = innermost_block.block ();
> +      var->root->valid_block =
> +	var->root->floating ? NULL : innermost_block.block ();
>        var->name = expression;
>        /* For a root var, the name and the expr are the same.  */
>        var->path_expr = expression;
> 

This LGTM.

Simon

[PATCHv5 0/5] Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

Sorry for the long delay between revisions.

This revision addresses the comments from Simon (thanks).

I've rebased the whole series onto the current upstream master, a
summary of my changes are as follows:

  #1 - Rewrite the commit message to address Simon's feedback.  Code
       itself is unchanged.

  #2 - Added 'm_' prefix to private variable, code is otherwise
       unchanged.

  #3 - Updated inline with feedback, commit message cleaned up, new
       test script cleaned up.

  #4 - No changes.

  #5 - No changes.

Pedro: Simon requested that you sign-off on the changes in patch #3,
  to make sure you are happy w.r.t. the comments you made in this
  message: https://sourceware.org/ml/gdb-patches/2016-08/msg00122.html

Thanks,
Andrew


---

Andrew Burgess (5):
  gdb: Remove duplicate declaration of global innermost_block
  gdb: New API for tracking innermost block
  gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up
  gdb: Remove out of date comment
  gdb: Don't store a thread-id for floating varobj

 gdb/ChangeLog                               |  54 ++++++++
 gdb/ada-exp.y                               |   6 +-
 gdb/ada-lang.c                              |   8 +-
 gdb/breakpoint.c                            |  12 +-
 gdb/c-exp.y                                 |  20 +--
 gdb/d-exp.y                                 |  11 +-
 gdb/expression.h                            |   5 -
 gdb/f-exp.y                                 |   7 +-
 gdb/go-exp.y                                |   7 +-
 gdb/m2-exp.y                                |  14 +--
 gdb/objfiles.c                              |   2 +-
 gdb/p-exp.y                                 |  12 +-
 gdb/parse.c                                 |  18 ++-
 gdb/parser-defs.h                           |  54 +++++++-
 gdb/printcmd.c                              |   8 +-
 gdb/rust-exp.y                              |   8 +-
 gdb/symfile.c                               |   2 +-
 gdb/testsuite/ChangeLog                     |  14 +++
 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/testsuite/gdb.python/py-mi.exp          |  12 +-
 gdb/varobj.c                                |  10 +-
 23 files changed, 371 insertions(+), 106 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp

-- 
2.14.3

[PATCHv5 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

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<enum innermost_block_tracker::track_type>
+    (static_cast<int> (a) | static_cast<int> (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 <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
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
-- 
2.14.3

[PATCHv5 1/5] gdb: Remove duplicate declaration of global innermost_block

[Andrew Burgess]

The global 'innermost_block' is declared in two header files.  Remove
one of the declarations, and add an include of the other header into
the one source file that could no longer see a declaration of
'innermost_block'.

gdb/ChangeLog:

	* expression.h (innermost_block): Remove declaration.
	* varobj.c: Add 'parser-defs.h' include.
---
 gdb/ChangeLog    | 5 +++++
 gdb/expression.h | 5 -----
 gdb/varobj.c     | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/expression.h b/gdb/expression.h
index a783ea5fef2..030f2f08e7a 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -111,11 +111,6 @@ extern expression_up parse_exp_1 (const char **, CORE_ADDR pc,
    attempt completion.  */
 extern int parse_completion;
 
-/* The innermost context required by the stack and register variables
-   we've encountered so far.  To use this, set it to NULL, then call
-   parse_<whatever>, then look at it.  */
-extern const struct block *innermost_block;
-
 /* From eval.c */
 
 /* Values of NOSIDE argument to eval_subexp.  */
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 01dabef37c3..701ef663766 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -30,6 +30,7 @@
 #include "gdbthread.h"
 #include "inferior.h"
 #include "varobj-iter.h"
+#include "parser-defs.h"
 
 #if HAVE_PYTHON
 #include "python/python.h"
-- 
2.14.3

[PATCHv5 5/5] gdb: Don't store a thread-id for floating varobj

[Andrew Burgess]

When creating a varobj with -var-create a user can create either fixed
varobj, or floating varobj.

A fixed varobj will always be evaluated within the thread/frame/block in
which the varobj was created, if that thread/frame/block is no longer
available then the varobj is considered out of scope.

A floating varobj will always be evaluated within the current
thread/frame/block.

Despite never using them GDB was storing the thread/frame/block into a
floating varobj, and the thread-id would then be displayed when GDB
reported on the state of the varobj, this could confuse a user into
thinking that the thread-id was relevant.

This commit prevents GDB storing the thread/frame/block onto floating
varobj, and updates the few tests where this impacts the results.

gdb/ChangeLog:

	* varobj.c (varobj_create): Don't set valid_block when creating a
	floating varobj.

gdb/testsuite/ChangeLog:

	* gdb.python/py-mi.exp: Don't expect a thread-id for floating
	varobj.
	* gdb.mi/mi-var-create-rtti.exp: Likewise.
---
 gdb/ChangeLog                               |  5 +++++
 gdb/testsuite/ChangeLog                     |  6 ++++++
 gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |  2 +-
 gdb/testsuite/gdb.python/py-mi.exp          | 12 ++++++------
 gdb/varobj.c                                |  3 ++-
 5 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index 9ea5784bcad..a5310ecc819 100644
--- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
@@ -52,6 +52,6 @@ mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
 	    "\\^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\"" \
+	    "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
 	    "-var-create sp2 @ \$sp"
 gdb_exit
diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index 0ecc5dfa13d..bbe1266724a 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -101,7 +101,7 @@ mi_varobj_update_dynamic container "varobj update 1" {
     type_changed false new_num_children 1 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
 }
 
 mi_next "next over update 2"
@@ -110,7 +110,7 @@ mi_varobj_update_dynamic container "varobj update 2" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container None" \
@@ -129,8 +129,8 @@ mi_varobj_update_dynamic container "varobj update after choosing default" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container ContainerPrinter" \
@@ -142,8 +142,8 @@ mi_varobj_update_dynamic container \
       type_changed false new_num_children 2 dynamic 1 has_more 0
   } {
   } {
-      { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-      { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+      { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+      { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
   }
 
 mi_list_varobj_children_range container 1 2 2 {
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 7d3800b2a48..97d9f5b6c5c 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -336,7 +336,8 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var.get ());
-      var->root->valid_block = innermost_block.block ();
+      var->root->valid_block =
+	var->root->floating ? NULL : innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
-- 
2.14.3

[PATCHv5 4/5] gdb: Remove out of date comment

[Andrew Burgess]

Comment clean up.

gdb/ChangeLog:

	* varobj.c (varobj_create): Remove out of date comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/varobj.c  | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 586bf5fc53b..7d3800b2a48 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -298,7 +298,6 @@ varobj_create (const char *objname,
       else
 	fi = NULL;
 
-      /* frame = -2 means always use selected frame.  */
       if (type == USE_SELECTED_FRAME)
 	var->root->floating = true;
 
-- 
2.14.3

[PATCHv5 2/5] gdb: New API for tracking innermost block

[Andrew Burgess]

This commit is preparation for a later change, at this point there
should be no user visible change.

We currently maintain a global innermost_block which tracks the most
inner block encountered when parsing an expression.

This commit wraps the innermost_block into a new class, and switches all
direct accesses to the variable to use the class API.

gdb/ChangeLog:

	* ada-exp.y (write_var_from_sym): Switch to innermost_block API.
	* ada-lang.c (resolve_subexp): Likewise.
	* breakpoint.c (set_breakpoint_condition) Likewise.
	(watch_command_1) Likewise.
	* c-exp.y (variable): Likewise.
	* d-exp.y (PrimaryExpression): Likewise.
	* f-exp.y (variable): Likewise.
	* go-exp.y (variable): Likewise.
	* m2-exp.y (variable): Likewise.
	* objfiles.c (objfile::~objfile): Likewise.
	* p-exp.y (variable): Likewise.
	* parse.c (innermost_block): Change type.
	* parser-defs.h (class innermost_block_tracker): New.
	(innermost_block): Change to innermost_block_tracker.
	* printcmd.c (display_command): Switch to innermost_block API.
	(do_one_display): Likewise.
	* rust-exp.y (do_one_display): Likewise.
	* symfile.c (clear_symtab_users): Likewise.
	* varobj.c (varobj_create): Switch to innermost_block API, replace
	use of innermost_block with block stored on varobj object.
---
 gdb/ChangeLog     | 23 +++++++++++++++++++++++
 gdb/ada-exp.y     |  6 +-----
 gdb/ada-lang.c    |  8 ++------
 gdb/breakpoint.c  | 12 ++++++------
 gdb/c-exp.y       | 20 ++++----------------
 gdb/d-exp.y       | 11 ++---------
 gdb/f-exp.y       |  7 +------
 gdb/go-exp.y      |  7 +------
 gdb/m2-exp.y      | 14 ++------------
 gdb/objfiles.c    |  2 +-
 gdb/p-exp.y       | 12 ++----------
 gdb/parse.c       | 13 ++++++++++++-
 gdb/parser-defs.h | 36 ++++++++++++++++++++++++++++++++++--
 gdb/printcmd.c    |  8 ++++----
 gdb/rust-exp.y    |  8 +++-----
 gdb/symfile.c     |  2 +-
 gdb/varobj.c      |  6 +++---
 17 files changed, 102 insertions(+), 93 deletions(-)

diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 0e6010816b5..fa7a4d5b4e8 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -757,11 +757,7 @@ write_var_from_sym (struct parser_state *par_state,
 		    struct symbol *sym)
 {
   if (symbol_read_needs_frame (sym))
-    {
-      if (innermost_block == 0
-	  || contained_in (block, innermost_block))
-	innermost_block = block;
-    }
+    innermost_block.update (block);
 
   write_exp_elt_opcode (par_state, OP_VAR_VALUE);
   write_exp_elt_block (par_state, block);
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 851e69ac4ba..481a29faad2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3507,9 +3507,7 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,
 
           exp->elts[pc + 1].block = candidates[i].block;
           exp->elts[pc + 2].symbol = candidates[i].symbol;
-          if (innermost_block == NULL
-              || contained_in (candidates[i].block, innermost_block))
-            innermost_block = candidates[i].block;
+	  innermost_block.update (candidates[i]);
         }
 
       if (deprocedure_p
@@ -3554,9 +3552,7 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,
 
             exp->elts[pc + 4].block = candidates[i].block;
             exp->elts[pc + 5].symbol = candidates[i].symbol;
-            if (innermost_block == NULL
-                || contained_in (candidates[i].block, innermost_block))
-              innermost_block = candidates[i].block;
+	    innermost_block.update (candidates[i]);
           }
       }
       break;
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2b5eebbbee8..91ecca62fc6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -879,12 +879,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  arg = exp;
 	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
 	  if (*arg)
 	    error (_("Junk at end of expression"));
-	  w->cond_exp_valid_block = innermost_block;
+	  w->cond_exp_valid_block = innermost_block.block ();
 	}
       else
 	{
@@ -10717,7 +10717,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   /* Parse the rest of the arguments.  From here on out, everything
      is in terms of a newly allocated string instead of the original
      ARG.  */
-  innermost_block = NULL;
+  innermost_block.reset ();
   std::string expression (arg, exp_end - arg);
   exp_start = arg = expression.c_str ();
   expression_up exp = parse_exp_1 (&arg, 0, 0, 0);
@@ -10739,7 +10739,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       error (_("Cannot watch constant value `%.*s'."), len, exp_start);
     }
 
-  exp_valid_block = innermost_block;
+  exp_valid_block = innermost_block.block ();
   mark = value_mark ();
   fetch_subexp_value (exp.get (), &pc, &val, &result, NULL, just_location);
 
@@ -10777,13 +10777,13 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   toklen = end_tok - tok;
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
-      innermost_block = NULL;
+      innermost_block.reset ();
       tok = cond_start = end_tok + 1;
       parse_exp_1 (&tok, 0, 0, 0);
 
       /* The watchpoint expression may not be local, but the condition
 	 may still be.  E.g.: `watch global if local > 0'.  */
-      cond_exp_valid_block = innermost_block;
+      cond_exp_valid_block = innermost_block.block ();
 
       cond_end = tok;
     }
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 8be13bf9d0c..0482e85ce80 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -949,12 +949,8 @@ variable:	block COLONCOLON name
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -1043,12 +1039,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -1060,10 +1051,7 @@ variable:	name_not_typename
 			      /* C++: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index 05b95d5b90d..03be93fbbc7 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -422,12 +422,7 @@ PrimaryExpression:
 		  if (sym.symbol && SYMBOL_CLASS (sym.symbol) != LOC_TYPEDEF)
 		    {
 		      if (symbol_read_needs_frame (sym.symbol))
-			{
-			  if (innermost_block == 0
-			      || contained_in (sym.block, innermost_block))
-			    innermost_block = sym.block;
-			}
-
+			innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 		      write_exp_elt_block (pstate, sym.block);
 		      write_exp_elt_sym (pstate, sym.symbol);
@@ -437,9 +432,7 @@ PrimaryExpression:
 		     {
 		      /* It hangs off of `this'.  Must not inadvertently convert from a
 			 method call to data ref.  */
-		      if (innermost_block == 0
-			  || contained_in (sym.block, innermost_block))
-			innermost_block = sym.block;
+		      innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index 6495e03cc55..ffd52cf3b1d 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -461,12 +461,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
 			      write_exp_elt_sym (pstate, sym.symbol);
diff --git a/gdb/go-exp.y b/gdb/go-exp.y
index 2eb69d1c1dd..a96e65534fd 100644
--- a/gdb/go-exp.y
+++ b/gdb/go-exp.y
@@ -552,12 +552,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 9e74a9d3334..2cf026c77a5 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -548,12 +548,7 @@ variable:	block COLONCOLON NAME
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -574,12 +569,7 @@ variable:	NAME
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0 ||
-				      contained_in (sym.block,
-						    innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 7adaef119d1..70e369b8b4f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -704,7 +704,7 @@ objfile::~objfile ()
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Check to see if the current_source_symtab belongs to this objfile,
      and if so, call clear_current_source_symtab_and_line.  */
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index 95a6924adfc..6b857e1a023 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -709,12 +709,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -728,10 +723,7 @@ variable:	name_not_typename
 			      /* Object pascal: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/parse.c b/gdb/parse.c
index 8b2bb22c76e..042df51bf8a 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -68,7 +68,7 @@ const struct exp_descriptor exp_descriptor_standard =
 /* Global variables declared in parser-defs.h (and commented there).  */
 const struct block *expression_context_block;
 CORE_ADDR expression_context_pc;
-const struct block *innermost_block;
+innermost_block_tracker innermost_block;
 int arglist_len;
 static struct type_stack type_stack;
 const char *lexptr;
@@ -121,6 +121,17 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
 					     const struct block *, int,
 					     int, int *);
 
+
+/* Update the stored innermost block if the new block is more inner than
+   the currently stored block, or if no block is stored yet.  */
+
+void
+innermost_block_tracker::update (const struct block *b)
+{
+  if (m_innermost_block == NULL || contained_in (b, m_innermost_block))
+    m_innermost_block = b;
+}
+
 /* Data structure for saving values of arglist_len for function calls whose
    arguments contain other function calls.  */
 
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index c537ed4bf7d..5265b394bea 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -75,9 +75,41 @@ extern const struct block *expression_context_block;
    then look up the macro definitions active at that point.  */
 extern CORE_ADDR expression_context_pc;
 
+/* When parsing expressions we track the innermost block that was
+   referenced.  These functions are described in more detail at their
+   definition site.  */
+class innermost_block_tracker
+{
+public:
+  innermost_block_tracker ()
+    : m_innermost_block (NULL)
+  { /* Nothing.  */ }
+
+  void reset ()
+  {
+    m_innermost_block = NULL;
+  }
+
+  void update (const struct block *b);
+
+  void update (const struct block_symbol &bs)
+  {
+    update (bs.block);
+  }
+
+  const struct block *block () const
+  {
+    return m_innermost_block;
+  }
+
+private:
+  const struct block *m_innermost_block;
+};
+
 /* The innermost context required by the stack and register variables
-   we've encountered so far.  */
-extern const struct block *innermost_block;
+   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;
 
 /* Number of arguments seen so far in innermost function call.  */
 extern int arglist_len;
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 6256f35baa5..fc9d7e4dd9a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1723,14 +1723,14 @@ display_command (const char *arg, int from_tty)
       fmt.raw = 0;
     }
 
-  innermost_block = NULL;
+  innermost_block.reset ();
   expression_up expr = parse_expression (exp);
 
   newobj = new display ();
 
   newobj->exp_string = xstrdup (exp);
   newobj->exp = std::move (expr);
-  newobj->block = innermost_block;
+  newobj->block = innermost_block.block ();
   newobj->pspace = current_program_space;
   newobj->number = ++display_number;
   newobj->format = fmt;
@@ -1891,9 +1891,9 @@ do_one_display (struct display *d)
 
       TRY
 	{
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  d->exp = parse_expression (d->exp_string);
-	  d->block = innermost_block;
+	  d->block = innermost_block.block ();
 	}
       CATCH (ex, RETURN_MASK_ALL)
 	{
diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y
index 199e87671ee..dcc5fc78eda 100644
--- a/gdb/rust-exp.y
+++ b/gdb/rust-exp.y
@@ -1044,15 +1044,13 @@ super_name (const struct rust_op *ident, unsigned int n_supers)
 		   ident->right.params);
 }
 
-/* A helper that updates innermost_block as appropriate.  */
+/* A helper that updates the innermost block as appropriate.  */
 
 static void
 update_innermost_block (struct block_symbol sym)
 {
-  if (symbol_read_needs_frame (sym.symbol)
-      && (innermost_block == NULL
-	  || contained_in (sym.block, innermost_block)))
-    innermost_block = sym.block;
+  if (symbol_read_needs_frame (sym.symbol))
+    innermost_block.update (sym);
 }
 
 /* A helper to look up a Rust type, or fail.  This only works for
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f7f75b05d32..ab6ec1cdadf 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2900,7 +2900,7 @@ clear_symtab_users (symfile_add_flags add_flags)
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Varobj may refer to old symbols, perform a cleanup.  */
   varobj_invalidate ();
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 701ef663766..20dd09bd585 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -311,7 +311,7 @@ varobj_create (const char *objname,
 	}
 
       p = expression;
-      innermost_block = NULL;
+      innermost_block.reset ();
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
@@ -336,7 +336,7 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var.get ());
-      var->root->valid_block = innermost_block;
+      var->root->valid_block = innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
@@ -345,7 +345,7 @@ varobj_create (const char *objname,
          we must select the appropriate frame before parsing
          the expression, otherwise the value will not be current.
          Since select_frame is so benign, just call it for all cases.  */
-      if (innermost_block)
+      if (var->root->valid_block)
 	{
 	  /* User could specify explicit FRAME-ADDR which was not found but
 	     EXPRESSION is frame specific and we would not be able to evaluate
-- 
2.14.3

Re: [PATCHv5 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Pedro Alves]

On 01/02/2018 03:31 PM, Andrew Burgess wrote:
> 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.

I think using DEF_ENUM_FLAGS_TYPE would be better, but I can live
with this too.

Note: I have a patch series from last year on my github that improves
DEF_ENUM_FLAGS_TYPE such that you can use it within a namespace, and
also with "enum class", IIRC.  Not sure it'll make the "nested
within class" case work, haven't looked at it in months:

 https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

I'll try to resurrect that series in this cycle.

> 
> Otherwise, I think all of the review comments have been addressed.

Yes, I think so.  This all looks good to me, with a couple nits:

>  public:
> +  enum track_type
> +    {
> +     for_symbols = 0x1,
> +     for_registers = 0x2
> +    };
> +

This new enum needs documentation.  Also each enumerator.
What do they mean?  Can one combine them?  Etc.

IMO, innermost_block_tracker's method description comments
would be better on the declarations than the definitions,
so that one can read the class in the header and better
understand the API by reading the public/client interface,
but I won't insist on that.

Thanks much for following through with this, and thanks
for the patience.

Pedro Alves

[PATCHv6 4/5] gdb: Remove out of date comment

[Andrew Burgess]

Comment clean up.

gdb/ChangeLog:

	* varobj.c (varobj_create): Remove out of date comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/varobj.c  | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 74789a1abae..23b386e6b07 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* varobj.c (varobj_create): Remove out of date comment.
+
 2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR mi/20395
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 6c0145402ab..523f74613bf 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -298,7 +298,6 @@ varobj_create (const char *objname,
       else
 	fi = NULL;
 
-      /* frame = -2 means always use selected frame.  */
       if (type == USE_SELECTED_FRAME)
 	var->root->floating = true;
 
-- 
2.14.3

[PATCHv6 2/5] gdb: New API for tracking innermost block

[Andrew Burgess]

This commit is preparation for a later change, at this point there
should be no user visible change.

We currently maintain a global innermost_block which tracks the most
inner block encountered when parsing an expression.

This commit wraps the innermost_block into a new class, and switches all
direct accesses to the variable to use the class API.

gdb/ChangeLog:

	* ada-exp.y (write_var_from_sym): Switch to innermost_block API.
	* ada-lang.c (resolve_subexp): Likewise.
	* breakpoint.c (set_breakpoint_condition) Likewise.
	(watch_command_1) Likewise.
	* c-exp.y (variable): Likewise.
	* d-exp.y (PrimaryExpression): Likewise.
	* f-exp.y (variable): Likewise.
	* go-exp.y (variable): Likewise.
	* m2-exp.y (variable): Likewise.
	* objfiles.c (objfile::~objfile): Likewise.
	* p-exp.y (variable): Likewise.
	* parse.c (innermost_block): Change type.
	* parser-defs.h (class innermost_block_tracker): New.
	(innermost_block): Change to innermost_block_tracker.
	* printcmd.c (display_command): Switch to innermost_block API.
	(do_one_display): Likewise.
	* rust-exp.y (do_one_display): Likewise.
	* symfile.c (clear_symtab_users): Likewise.
	* varobj.c (varobj_create): Switch to innermost_block API, replace
	use of innermost_block with block stored on varobj object.
---
 gdb/ChangeLog     | 23 +++++++++++++++++++++++
 gdb/ada-exp.y     |  6 +-----
 gdb/ada-lang.c    |  8 ++------
 gdb/breakpoint.c  | 12 ++++++------
 gdb/c-exp.y       | 20 ++++----------------
 gdb/d-exp.y       | 11 ++---------
 gdb/f-exp.y       |  7 +------
 gdb/go-exp.y      |  7 +------
 gdb/m2-exp.y      | 14 ++------------
 gdb/objfiles.c    |  2 +-
 gdb/p-exp.y       | 12 ++----------
 gdb/parse.c       | 11 ++++++++++-
 gdb/parser-defs.h | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 gdb/printcmd.c    |  8 ++++----
 gdb/rust-exp.y    |  8 +++-----
 gdb/symfile.c     |  2 +-
 gdb/varobj.c      |  6 +++---
 17 files changed, 111 insertions(+), 94 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7d7d10661ea..13733ea00e8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,26 @@
+2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* ada-exp.y (write_var_from_sym): Switch to innermost_block API.
+	* ada-lang.c (resolve_subexp): Likewise.
+	* breakpoint.c (set_breakpoint_condition) Likewise.
+	(watch_command_1) Likewise.
+	* c-exp.y (variable): Likewise.
+	* d-exp.y (PrimaryExpression): Likewise.
+	* f-exp.y (variable): Likewise.
+	* go-exp.y (variable): Likewise.
+	* m2-exp.y (variable): Likewise.
+	* objfiles.c (objfile::~objfile): Likewise.
+	* p-exp.y (variable): Likewise.
+	* parse.c (innermost_block): Change type.
+	* parser-defs.h (class innermost_block_tracker): New.
+	(innermost_block): Change to innermost_block_tracker.
+	* printcmd.c (display_command): Switch to innermost_block API.
+	(do_one_display): Likewise.
+	* rust-exp.y (do_one_display): Likewise.
+	* symfile.c (clear_symtab_users): Likewise.
+	* varobj.c (varobj_create): Switch to innermost_block API, replace
+	use of innermost_block with block stored on varobj object.
+
 2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* expression.h (innermost_block): Remove declaration.
diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 0acd1e287e5..56113186b97 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -757,11 +757,7 @@ write_var_from_sym (struct parser_state *par_state,
 		    struct symbol *sym)
 {
   if (symbol_read_needs_frame (sym))
-    {
-      if (innermost_block == 0
-	  || contained_in (block, innermost_block))
-	innermost_block = block;
-    }
+    innermost_block.update (block);
 
   write_exp_elt_opcode (par_state, OP_VAR_VALUE);
   write_exp_elt_block (par_state, block);
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index ab1083830ed..3ff71691248 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3507,9 +3507,7 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,
 
           exp->elts[pc + 1].block = candidates[i].block;
           exp->elts[pc + 2].symbol = candidates[i].symbol;
-          if (innermost_block == NULL
-              || contained_in (candidates[i].block, innermost_block))
-            innermost_block = candidates[i].block;
+	  innermost_block.update (candidates[i]);
         }
 
       if (deprocedure_p
@@ -3554,9 +3552,7 @@ resolve_subexp (expression_up *expp, int *pos, int deprocedure_p,
 
             exp->elts[pc + 4].block = candidates[i].block;
             exp->elts[pc + 5].symbol = candidates[i].symbol;
-            if (innermost_block == NULL
-                || contained_in (candidates[i].block, innermost_block))
-              innermost_block = candidates[i].block;
+	    innermost_block.update (candidates[i]);
           }
       }
       break;
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2b5eebbbee8..91ecca62fc6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -879,12 +879,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  arg = exp;
 	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
 	  if (*arg)
 	    error (_("Junk at end of expression"));
-	  w->cond_exp_valid_block = innermost_block;
+	  w->cond_exp_valid_block = innermost_block.block ();
 	}
       else
 	{
@@ -10717,7 +10717,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   /* Parse the rest of the arguments.  From here on out, everything
      is in terms of a newly allocated string instead of the original
      ARG.  */
-  innermost_block = NULL;
+  innermost_block.reset ();
   std::string expression (arg, exp_end - arg);
   exp_start = arg = expression.c_str ();
   expression_up exp = parse_exp_1 (&arg, 0, 0, 0);
@@ -10739,7 +10739,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
       error (_("Cannot watch constant value `%.*s'."), len, exp_start);
     }
 
-  exp_valid_block = innermost_block;
+  exp_valid_block = innermost_block.block ();
   mark = value_mark ();
   fetch_subexp_value (exp.get (), &pc, &val, &result, NULL, just_location);
 
@@ -10777,13 +10777,13 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   toklen = end_tok - tok;
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
-      innermost_block = NULL;
+      innermost_block.reset ();
       tok = cond_start = end_tok + 1;
       parse_exp_1 (&tok, 0, 0, 0);
 
       /* The watchpoint expression may not be local, but the condition
 	 may still be.  E.g.: `watch global if local > 0'.  */
-      cond_exp_valid_block = innermost_block;
+      cond_exp_valid_block = innermost_block.block ();
 
       cond_end = tok;
     }
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 8be13bf9d0c..0482e85ce80 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -949,12 +949,8 @@ variable:	block COLONCOLON name
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -1043,12 +1039,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -1060,10 +1051,7 @@ variable:	name_not_typename
 			      /* C++: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index 05b95d5b90d..03be93fbbc7 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -422,12 +422,7 @@ PrimaryExpression:
 		  if (sym.symbol && SYMBOL_CLASS (sym.symbol) != LOC_TYPEDEF)
 		    {
 		      if (symbol_read_needs_frame (sym.symbol))
-			{
-			  if (innermost_block == 0
-			      || contained_in (sym.block, innermost_block))
-			    innermost_block = sym.block;
-			}
-
+			innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 		      write_exp_elt_block (pstate, sym.block);
 		      write_exp_elt_sym (pstate, sym.symbol);
@@ -437,9 +432,7 @@ PrimaryExpression:
 		     {
 		      /* It hangs off of `this'.  Must not inadvertently convert from a
 			 method call to data ref.  */
-		      if (innermost_block == 0
-			  || contained_in (sym.block, innermost_block))
-			innermost_block = sym.block;
+		      innermost_block.update (sym);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, OP_THIS);
 		      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index 6495e03cc55..ffd52cf3b1d 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -461,12 +461,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
 			      write_exp_elt_sym (pstate, sym.symbol);
diff --git a/gdb/go-exp.y b/gdb/go-exp.y
index 2eb69d1c1dd..a96e65534fd 100644
--- a/gdb/go-exp.y
+++ b/gdb/go-exp.y
@@ -552,12 +552,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 9e74a9d3334..2cf026c77a5 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -548,12 +548,7 @@ variable:	block COLONCOLON NAME
 			    error (_("No symbol \"%s\" in specified context."),
 				   copy_name ($3));
 			  if (symbol_read_needs_frame (sym.symbol))
-			    {
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
-			    }
+			    innermost_block.update (sym);
 
 			  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			  write_exp_elt_block (pstate, sym.block);
@@ -574,12 +569,7 @@ variable:	NAME
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0 ||
-				      contained_in (sym.block,
-						    innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 7adaef119d1..70e369b8b4f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -704,7 +704,7 @@ objfile::~objfile ()
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Check to see if the current_source_symtab belongs to this objfile,
      and if so, call clear_current_source_symtab_and_line.  */
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index 95a6924adfc..6b857e1a023 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -709,12 +709,7 @@ variable:	name_not_typename
 			  if (sym.symbol)
 			    {
 			      if (symbol_read_needs_frame (sym.symbol))
-				{
-				  if (innermost_block == 0
-				      || contained_in (sym.block,
-						       innermost_block))
-				    innermost_block = sym.block;
-				}
+				innermost_block.update (sym);
 
 			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
 			      write_exp_elt_block (pstate, sym.block);
@@ -728,10 +723,7 @@ variable:	name_not_typename
 			      /* Object pascal: it hangs off of `this'.  Must
 			         not inadvertently convert from a method call
 				 to data ref.  */
-			      if (innermost_block == 0
-				  || contained_in (sym.block,
-						   innermost_block))
-				innermost_block = sym.block;
+			      innermost_block.update (sym);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, OP_THIS);
 			      write_exp_elt_opcode (pstate, STRUCTOP_PTR);
diff --git a/gdb/parse.c b/gdb/parse.c
index 8b2bb22c76e..ca5eb023869 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -68,7 +68,7 @@ const struct exp_descriptor exp_descriptor_standard =
 /* Global variables declared in parser-defs.h (and commented there).  */
 const struct block *expression_context_block;
 CORE_ADDR expression_context_pc;
-const struct block *innermost_block;
+innermost_block_tracker innermost_block;
 int arglist_len;
 static struct type_stack type_stack;
 const char *lexptr;
@@ -121,6 +121,15 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
 					     const struct block *, int,
 					     int, int *);
 
+/* Documented at it's declaration.  */
+
+void
+innermost_block_tracker::update (const struct block *b)
+{
+  if (m_innermost_block == NULL || contained_in (b, m_innermost_block))
+    m_innermost_block = b;
+}
+
 /* Data structure for saving values of arglist_len for function calls whose
    arguments contain other function calls.  */
 
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index c537ed4bf7d..01ac0cd363a 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -75,9 +75,51 @@ extern const struct block *expression_context_block;
    then look up the macro definitions active at that point.  */
 extern CORE_ADDR expression_context_pc;
 
-/* The innermost context required by the stack and register variables
-   we've encountered so far.  */
-extern const struct block *innermost_block;
+/* When parsing expressions we track the innermost block that was
+   referenced.  */
+
+class innermost_block_tracker
+{
+public:
+  innermost_block_tracker ()
+    : m_innermost_block (NULL)
+  { /* Nothing.  */ }
+
+  /* Reset the currently stored innermost block.  Usually called before
+     parsing a new expression.  */
+  void reset ()
+  {
+    m_innermost_block = nullptr;
+  }
+
+  /* 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);
+
+  /* Overload of main UPDATE method which extracts the block from BS.  */
+  void update (const struct block_symbol &bs)
+  {
+    update (bs.block);
+  }
+
+  /* Return the stored innermost block.  Can be nullptr if no symbols or
+     registers were found during an expression parse, and so no innermost
+     block was defined.  */
+  const struct block *block () const
+  {
+    return m_innermost_block;
+  }
+
+private:
+  /* 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
+   expression, and queried once the parse is complete.  */
+extern innermost_block_tracker innermost_block;
 
 /* Number of arguments seen so far in innermost function call.  */
 extern int arglist_len;
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 6256f35baa5..fc9d7e4dd9a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1723,14 +1723,14 @@ display_command (const char *arg, int from_tty)
       fmt.raw = 0;
     }
 
-  innermost_block = NULL;
+  innermost_block.reset ();
   expression_up expr = parse_expression (exp);
 
   newobj = new display ();
 
   newobj->exp_string = xstrdup (exp);
   newobj->exp = std::move (expr);
-  newobj->block = innermost_block;
+  newobj->block = innermost_block.block ();
   newobj->pspace = current_program_space;
   newobj->number = ++display_number;
   newobj->format = fmt;
@@ -1891,9 +1891,9 @@ do_one_display (struct display *d)
 
       TRY
 	{
-	  innermost_block = NULL;
+	  innermost_block.reset ();
 	  d->exp = parse_expression (d->exp_string);
-	  d->block = innermost_block;
+	  d->block = innermost_block.block ();
 	}
       CATCH (ex, RETURN_MASK_ALL)
 	{
diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y
index 199e87671ee..dcc5fc78eda 100644
--- a/gdb/rust-exp.y
+++ b/gdb/rust-exp.y
@@ -1044,15 +1044,13 @@ super_name (const struct rust_op *ident, unsigned int n_supers)
 		   ident->right.params);
 }
 
-/* A helper that updates innermost_block as appropriate.  */
+/* A helper that updates the innermost block as appropriate.  */
 
 static void
 update_innermost_block (struct block_symbol sym)
 {
-  if (symbol_read_needs_frame (sym.symbol)
-      && (innermost_block == NULL
-	  || contained_in (sym.block, innermost_block)))
-    innermost_block = sym.block;
+  if (symbol_read_needs_frame (sym.symbol))
+    innermost_block.update (sym);
 }
 
 /* A helper to look up a Rust type, or fail.  This only works for
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f7f75b05d32..ab6ec1cdadf 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2900,7 +2900,7 @@ clear_symtab_users (symfile_add_flags add_flags)
      FIXME: It's not clear which of these are supposed to persist
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
-  innermost_block = NULL;
+  innermost_block.reset ();
 
   /* Varobj may refer to old symbols, perform a cleanup.  */
   varobj_invalidate ();
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 701ef663766..20dd09bd585 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -311,7 +311,7 @@ varobj_create (const char *objname,
 	}
 
       p = expression;
-      innermost_block = NULL;
+      innermost_block.reset ();
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
@@ -336,7 +336,7 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var.get ());
-      var->root->valid_block = innermost_block;
+      var->root->valid_block = innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
@@ -345,7 +345,7 @@ varobj_create (const char *objname,
          we must select the appropriate frame before parsing
          the expression, otherwise the value will not be current.
          Since select_frame is so benign, just call it for all cases.  */
-      if (innermost_block)
+      if (var->root->valid_block)
 	{
 	  /* User could specify explicit FRAME-ADDR which was not found but
 	     EXPRESSION is frame specific and we would not be able to evaluate
-- 
2.14.3

[PATCHv6 1/5] gdb: Remove duplicate declaration of global innermost_block

[Andrew Burgess]

The global 'innermost_block' is declared in two header files.  Remove
one of the declarations, and add an include of the other header into
the one source file that could no longer see a declaration of
'innermost_block'.

gdb/ChangeLog:

	* expression.h (innermost_block): Remove declaration.
	* varobj.c: Add 'parser-defs.h' include.
---
 gdb/ChangeLog    | 5 +++++
 gdb/expression.h | 5 -----
 gdb/varobj.c     | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d2d44725a02..7d7d10661ea 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* expression.h (innermost_block): Remove declaration.
+	* varobj.c: Add 'parser-defs.h' include.
+
 2018-01-19  Tom Tromey  <tom@tromey.com>
 
 	* rust-lang.c (rust_lookup_symbol_nonlocal): Look up qualified
diff --git a/gdb/expression.h b/gdb/expression.h
index a783ea5fef2..030f2f08e7a 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -111,11 +111,6 @@ extern expression_up parse_exp_1 (const char **, CORE_ADDR pc,
    attempt completion.  */
 extern int parse_completion;
 
-/* The innermost context required by the stack and register variables
-   we've encountered so far.  To use this, set it to NULL, then call
-   parse_<whatever>, then look at it.  */
-extern const struct block *innermost_block;
-
 /* From eval.c */
 
 /* Values of NOSIDE argument to eval_subexp.  */
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 01dabef37c3..701ef663766 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -30,6 +30,7 @@
 #include "gdbthread.h"
 #include "inferior.h"
 #include "varobj-iter.h"
+#include "parser-defs.h"
 
 #if HAVE_PYTHON
 #include "python/python.h"
-- 
2.14.3

[PATCHv6 5/5] gdb: Don't store a thread-id for floating varobj

[Andrew Burgess]

When creating a varobj with -var-create a user can create either fixed
varobj, or floating varobj.

A fixed varobj will always be evaluated within the thread/frame/block in
which the varobj was created, if that thread/frame/block is no longer
available then the varobj is considered out of scope.

A floating varobj will always be evaluated within the current
thread/frame/block.

Despite never using them GDB was storing the thread/frame/block into a
floating varobj, and the thread-id would then be displayed when GDB
reported on the state of the varobj, this could confuse a user into
thinking that the thread-id was relevant.

This commit prevents GDB storing the thread/frame/block onto floating
varobj, and updates the few tests where this impacts the results.

gdb/ChangeLog:

	* varobj.c (varobj_create): Don't set valid_block when creating a
	floating varobj.

gdb/testsuite/ChangeLog:

	* gdb.python/py-mi.exp: Don't expect a thread-id for floating
	varobj.
	* gdb.mi/mi-var-create-rtti.exp: Likewise.
---
 gdb/ChangeLog                               |  5 +++++
 gdb/testsuite/ChangeLog                     |  6 ++++++
 gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |  2 +-
 gdb/testsuite/gdb.python/py-mi.exp          | 12 ++++++------
 gdb/varobj.c                                |  3 ++-
 5 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 23b386e6b07..16a7c683040 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* varobj.c (varobj_create): Don't set valid_block when creating a
+	floating varobj.
+
 2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* varobj.c (varobj_create): Remove out of date comment.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c65c4f7d328..93eb650c4d3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.python/py-mi.exp: Don't expect a thread-id for floating
+	varobj.
+	* gdb.mi/mi-var-create-rtti.exp: Likewise.
+
 2017-10-19  Don Breazeal  <donb@codesourcery.com>
 	    Andrew Burgess  <andrew.burgess@embecosm.com>
 
diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
index 9ea5784bcad..a5310ecc819 100644
--- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
@@ -52,6 +52,6 @@ mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
 	    "\\^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\"" \
+	    "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
 	    "-var-create sp2 @ \$sp"
 gdb_exit
diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index 0ecc5dfa13d..bbe1266724a 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -101,7 +101,7 @@ mi_varobj_update_dynamic container "varobj update 1" {
     type_changed false new_num_children 1 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
 }
 
 mi_next "next over update 2"
@@ -110,7 +110,7 @@ mi_varobj_update_dynamic container "varobj update 2" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container None" \
@@ -129,8 +129,8 @@ mi_varobj_update_dynamic container "varobj update after choosing default" {
     type_changed false new_num_children 2 dynamic 1 has_more 0
 } {
 } {
-    { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-    { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+    { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+    { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
 }
 
 mi_gdb_test "-var-set-visualizer container ContainerPrinter" \
@@ -142,8 +142,8 @@ mi_varobj_update_dynamic container \
       type_changed false new_num_children 2 dynamic 1 has_more 0
   } {
   } {
-      { name {container.\[0\]} exp {\[0\]} numchild 0 type int thread-id 1 }
-      { name {container.\[1\]} exp {\[1\]} numchild 0 type int thread-id 1 }
+      { name {container.\[0\]} exp {\[0\]} numchild 0 type int }
+      { name {container.\[1\]} exp {\[1\]} numchild 0 type int }
   }
 
 mi_list_varobj_children_range container 1 2 2 {
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 523f74613bf..b6a2d8f3696 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -336,7 +336,8 @@ varobj_create (const char *objname,
 	}
 
       var->format = variable_default_display (var.get ());
-      var->root->valid_block = innermost_block.block ();
+      var->root->valid_block =
+	var->root->floating ? NULL : innermost_block.block ();
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
       var->path_expr = expression;
-- 
2.14.3

[PATCHv6 0/5] Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

Updated to address Pedro's feedback.  Only changes are in patches #2
and #3 to increase the comments on the innermost_block_tracker class,
and to move the innermost_block_tracker_type enum out of the class and
switch to using DEF_ENUM_FLAGS_TYPE.

Thanks,
Andrew

---

Andrew Burgess (5):
  gdb: Remove duplicate declaration of global innermost_block
  gdb: New API for tracking innermost block
  gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up
  gdb: Remove out of date comment
  gdb: Don't store a thread-id for floating varobj

 gdb/ChangeLog                               |  54 ++++++++
 gdb/ada-exp.y                               |   6 +-
 gdb/ada-lang.c                              |   8 +-
 gdb/breakpoint.c                            |  12 +-
 gdb/c-exp.y                                 |  20 +--
 gdb/d-exp.y                                 |  11 +-
 gdb/expression.h                            |   5 -
 gdb/f-exp.y                                 |   7 +-
 gdb/go-exp.y                                |   7 +-
 gdb/m2-exp.y                                |  14 +--
 gdb/objfiles.c                              |   2 +-
 gdb/p-exp.y                                 |  12 +-
 gdb/parse.c                                 |  16 ++-
 gdb/parser-defs.h                           |  74 ++++++++++-
 gdb/printcmd.c                              |   8 +-
 gdb/rust-exp.y                              |   8 +-
 gdb/symfile.c                               |   2 +-
 gdb/testsuite/ChangeLog                     |  14 +++
 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/testsuite/gdb.python/py-mi.exp          |  12 +-
 gdb/varobj.c                                |  10 +-
 23 files changed, 389 insertions(+), 106 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp

-- 
2.14.3

[PATCHv6 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Andrew Burgess]

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                           |  46 +++++--
 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, 264 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 13733ea00e8..74789a1abae 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,20 @@
+2017-10-19  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::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.
+
 2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* ada-exp.y (write_var_from_sym): Switch to innermost_block API.
diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 56113186b97..ac4c341bfe9 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_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 ca5eb023869..e3f1306a175 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -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;
 }
 
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 01ac0cd363a..11865b42ba9 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -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 track 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,31 @@ 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, which was 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 +136,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;
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 2b968010fd5..c65c4f7d328 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2017-10-19  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-19  Tom Tromey  <tom@tromey.com>
 
 	* gdb.rust/modules.rs (TWENTY_THREE): New global.
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 <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
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..6c0145402ab 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_FOR_SYMBOLS
+			     | INNERMOST_BLOCK_FOR_REGISTERS);
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
-- 
2.14.3

Re: [PATCHv6 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up

[Pedro Alves]

Hi Andrew,

This version of the series all looks good to me.  I only have
comments in this patch.  Fix them, and you're good to go.
Please push.

On 01/20/2018 09:34 PM, Andrew Burgess wrote:

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 13733ea00e8..74789a1abae 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,20 @@
> +2017-10-19  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::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.
> +

ChangeLog needs updating.  (Here and in commit log.)

> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index 01ac0cd363a..11865b42ba9 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -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 track to control which blocks we consider for the
> +   innermost block, these flags can be combined together as needed.  */

The last sentence reads a bit weird to me.  I think you meant:

  "innermost block track" -> "innermost block tracker"

Maybe a period instead of a comma before "these flags"?

>  
>    /* 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, which was set during reset.  */

Period instead of comma before "the stored" ?

> +  void update (const struct block *b, innermost_block_tracker_types t);

Thanks,
Pedro Alves