[python][patch] Fix 11036

[python][patch] Fix 11036

[Phil Muldoon]

This patch fixes python/11036.  It allows access to variables that
have the same name in different scopes to be differentiated in
read_var with an optional block argument.  This allows the API user to
refine the read_var search.

What do you think?

Cheers,

Phil 

--

ChangeLog

2010-02-25  Phil Muldoon  <pmuldoon@redhat.com>

	PR python/11036
	* python/py-frame.c (frapy_read_var): Add block argument and logic
	to cope with user provided blocks.

doc/ChangeLog

2010-02-25  Phil Muldoon  <pmuldoon@redhat.com>

	* gdb.texinfo (Frames In Python): Add block parameter and
	description to read_var text.

testsuite/ChangeLog

2010-02-25  Phil Muldoon  <pmuldoon@redhat.com>

	* gdb.python/py-frame.exp: Add read_var block tests.
	* gdb.python/py-frame.c (block): New function.

--

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0a7191e..d8b7572 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2010-02-25  Phil Muldoon  <pmuldoon@redhat.com>
+
+	PR python/11036
+	* python/py-frame.c (frapy_read_var): Add block argument and logic
+	to cope with user provided blocks.
+
 2010-02-24  Pedro Alves  <pedro@codesourcery.com>
 
 	* mi/mi-main.c (mi_cmd_execute): Fix typo.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 82742d4..ded9aa2 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -20795,9 +20795,12 @@ Return the frame's symtab and line object.
 @xref{Symbol Tables In Python}.
 @end defmethod
 
-@defmethod Frame read_var variable
-Return the value of the given variable in this frame.  @var{variable} must
-be a string.
+@defmethod Frame read_var variable @r{[}block@r{]}
+Return the value of the given variable in this frame.  If the optional
+block argument is provided, search for the variable from that current
+block; otherwise start at the frame's current block (which is
+determined by the frame's current program counter).  @var{variable}
+must be a string.  @var{block} must be a @code{gdb.Block} object.
 @end defmethod
 
 @defmethod Frame select
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 94211db..46c0bb0 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -380,20 +380,23 @@ frapy_find_sal (PyObject *self, PyObject *args)
   return sal_obj;
 }
 
-/* Implementation of gdb.Frame.read_var_value (self, variable) -> gdb.Value.
-   Returns the value of the given variable in this frame.  The argument must be
-   a string.  Returns None if GDB can't find the specified variable.  */
-
+/* Implementation of gdb.Frame.read_var_value (self, variable,
+   [block]) -> gdb.Value.  If the optional block argument is provided
+   start the search from that block, otherwise search from the frame's
+   current block (determined by examining the resume address of the
+   frame).  The variable argument must be a string.  The block
+   argument must be an instance of gdb.Block.  Returns None if GDB
+   can't find the specified variable.  */
 static PyObject *
 frapy_read_var (PyObject *self, PyObject *args)
 {
   struct frame_info *frame;
-  PyObject *sym_obj;
+  PyObject *sym_obj, *block_obj = NULL;
   struct symbol *var = NULL;	/* gcc-4.3.2 false warning.  */
   struct value *val = NULL;
   volatile struct gdb_exception except;
 
-  if (!PyArg_ParseTuple (args, "O", &sym_obj))
+  if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj))
     return NULL;
 
   if (PyObject_TypeCheck (sym_obj, &symbol_object_type))
@@ -410,11 +413,19 @@ frapy_read_var (PyObject *self, PyObject *args)
 	return NULL;
       cleanup = make_cleanup (xfree, var_name);
 
+      if (block_obj)
+	{
+	  block = block_object_to_block (block_obj);
+	  if (block == NULL)
+	    return NULL;
+	}
+
       TRY_CATCH (except, RETURN_MASK_ALL)
 	{
 	  FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
 
-	  block = block_for_pc (get_frame_address_in_block (frame));
+	  if (!block)
+	    block = block_for_pc (get_frame_address_in_block (frame));
 	  var = lookup_symbol (var_name, block, VAR_DOMAIN, NULL);
 	}
       GDB_PY_HANDLE_EXCEPTION (except);
diff --git a/gdb/testsuite/gdb.python/py-frame.c b/gdb/testsuite/gdb.python/py-frame.c
index 22eb9f2..82db341 100644
--- a/gdb/testsuite/gdb.python/py-frame.c
+++ b/gdb/testsuite/gdb.python/py-frame.c
@@ -8,7 +8,23 @@ int f1 (int a, int b)
   return f2(a) + b;
 }
 
+int block (void)
+{
+  int i = 99;
+  {
+    double i = 1.1;
+    double f = 2.2;
+    {
+      const char *i = "stuff";
+      const char *f = "foo";
+      const char *b = "bar";
+      return 0; /* Block break here.  */
+    }
+  }
+}
+
 int main (int argc, char *argv[])
 {
+  block ();
   return f1 (1, 2);
 }
diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
index 6989207..150e737 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -56,6 +56,30 @@ if ![runto_main] then {
     return 0
 }
 
+gdb_breakpoint [gdb_get_line_number "Block break here."]
+gdb_continue_to_breakpoint "Block break here."
+gdb_py_test_silent_cmd "python bf1 = gdb.selected_frame ()" "get frame" 0
+
+# First test that read_var is unaffected by PR 11036 changes.
+gdb_test "python print bf1.read_var(\"i\")" "\"stuff\"" "test i"
+gdb_test "python print bf1.read_var(\"f\")" "\"foo\"" "test f"
+gdb_test "python print bf1.read_var(\"b\")" "\"bar\"" "test b"
+
+# Test the read_var function in another block other than the current
+# block (in this case, the super block). Test thar read_var is reading
+# the correct variables of i and f but they are the correct value and type.
+gdb_py_test_silent_cmd "python sb = bf1.block().superblock" "get superblock" 0
+gdb_test "python print bf1.read_var(\"i\", sb)" "1.1.*" "test i = 1.1"
+gdb_test "python print bf1.read_var(\"i\", sb).type" "double" "test double i"
+gdb_test "python print bf1.read_var(\"f\", sb)" "2.2.*" "test f = 2.2"
+gdb_test "python print bf1.read_var(\"f\", sb).type" "double" "test double f"
+
+# And again test another outerblock, this time testing "i" is the
+# correct value and type.
+gdb_py_test_silent_cmd "python sb = sb.superblock" "get superblock" 0
+gdb_test "python print bf1.read_var(\"i\", sb)" "99" "test i = 99"
+gdb_test "python print bf1.read_var(\"i\", sb).type" "int" "test int i"
+
 gdb_breakpoint "f2"
 gdb_continue_to_breakpoint "breakpoint at f2"
 gdb_test "up" "" ""

Re: [python][patch] Fix 11036

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> 2010-02-25  Phil Muldoon  <pmuldoon@redhat.com>
Phil> 	PR python/11036
Phil> 	* python/py-frame.c (frapy_read_var): Add block argument and logic
Phil> 	to cope with user provided blocks.

Phil> +   argument must be an instance of gdb.Block.  Returns None if GDB
Phil> +   can't find the specified variable.  */

This last sentence in this comment seems to be wrong.
Sometimes the function returns None, sometimes it throws an exception.
I think the function should behave more predictably somehow.

Phil> +	  block = block_object_to_block (block_obj);
Phil> +	  if (block == NULL)
Phil> +	    return NULL;

When block_object_to_block returns NULL, it does not set the Python
exception.  That makes this code erroneous.

In other respects the code bits seem reasonable to me.

Tom

Re: [python][patch] Fix 11036

[Phil Muldoon]

On Thu, Feb 25, 2010 at 11:33:49AM -0700, Tom Tromey wrote:
> >>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:


> Phil> +   argument must be an instance of gdb.Block.  Returns None if GDB
> Phil> +   can't find the specified variable.  */
> 
> This last sentence in this comment seems to be wrong.
> Sometimes the function returns None, sometimes it throws an exception.
> I think the function should behave more predictably somehow.


I took a look at the code that lies beneath the instance where this
function could return None.  It seems that read_var_value can
(according to the comments heading that function) return NULL.  I'm
stumped when this can happen, as it deals with each LOC_* scenario,
throwing the relevant errors when needed.  We talked a little about
this on irc last night, and even in the case of symbols where the value
have been optimized away, read_var still returns non-null:

(gdb) py f = gdb.selected_frame()
(gdb) py print f.read_var("optimize_me")
<value optimized out>

If you examine the runtime, the return here is a valid value, not
a NULL.  In any case, I think that if read_var returns NULL, then this
is an error case.  If anyone knows this to be different let me know and
I can go back to returning None in this case.  To reflect this, if the
result of read_var is NULL, I raise an exception.  What do you think?

> Phil> +	  block = block_object_to_block (block_obj);
> Phil> +	  if (block == NULL)
> Phil> +	    return NULL;

Ok, fixed.

I have also updated the comments and the documentation to reflect the
new decisions.

Cheers,

Phil

--

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f6105b7..23aca55 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -20799,9 +20799,13 @@ Return the frame's symtab and line object.
 @xref{Symbol Tables In Python}.
 @end defmethod
 
-@defmethod Frame read_var variable
-Return the value of the given variable in this frame.  @var{variable} must
-be a string.
+@defmethod Frame read_var variable @r{[}block@r{]}
+Return the value of the given variable in this frame.  If the optional
+block argument is provided, search for the variable from that current
+block; otherwise start at the frame's current block (which is
+determined by the frame's current program counter).  @var{variable}
+must be a string or a @code{gdb.Symbol} object.  @var{block} must be
+a @code{gdb.Block} object.
 @end defmethod
 
 @defmethod Frame select
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 94211db..3b5320c 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -380,20 +380,22 @@ frapy_find_sal (PyObject *self, PyObject *args)
   return sal_obj;
 }
 
-/* Implementation of gdb.Frame.read_var_value (self, variable) -> gdb.Value.
-   Returns the value of the given variable in this frame.  The argument must be
-   a string.  Returns None if GDB can't find the specified variable.  */
-
+/* Implementation of gdb.Frame.read_var_value (self, variable,
+   [block]) -> gdb.Value.  If the optional block argument is provided
+   start the search from that block, otherwise search from the frame's
+   current block (determined by examining the resume address of the
+   frame).  The variable argument must be a string or an instance of a
+   gdb.Symbol.  The block argument must be an instance of gdb.Block.  */
 static PyObject *
 frapy_read_var (PyObject *self, PyObject *args)
 {
   struct frame_info *frame;
-  PyObject *sym_obj;
+  PyObject *sym_obj, *block_obj = NULL;
   struct symbol *var = NULL;	/* gcc-4.3.2 false warning.  */
   struct value *val = NULL;
   volatile struct gdb_exception except;
 
-  if (!PyArg_ParseTuple (args, "O", &sym_obj))
+  if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj))
     return NULL;
 
   if (PyObject_TypeCheck (sym_obj, &symbol_object_type))
@@ -410,11 +412,23 @@ frapy_read_var (PyObject *self, PyObject *args)
 	return NULL;
       cleanup = make_cleanup (xfree, var_name);
 
+      if (block_obj)
+	{
+	  block = block_object_to_block (block_obj);
+	  if (!block)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Second argument must be block."));
+	      return NULL;
+	    }
+	}
+
       TRY_CATCH (except, RETURN_MASK_ALL)
 	{
 	  FRAPY_REQUIRE_VALID ((frame_object *) self, frame);
 
-	  block = block_for_pc (get_frame_address_in_block (frame));
+	  if (!block)
+	    block = block_for_pc (get_frame_address_in_block (frame));
 	  var = lookup_symbol (var_name, block, VAR_DOMAIN, NULL);
 	}
       GDB_PY_HANDLE_EXCEPTION (except);
@@ -445,10 +459,15 @@ frapy_read_var (PyObject *self, PyObject *args)
     }
   GDB_PY_HANDLE_EXCEPTION (except);
 
-  if (val)
-    return value_to_value_object (val);
+  if (!val)
+    {
+      PyErr_Format (PyExc_ValueError,
+		    _("Variable cannot be found for symbol '%s'."),
+		    SYMBOL_NATURAL_NAME (var));
+      return NULL;
+    }
 
-  Py_RETURN_NONE;
+  return value_to_value_object (val);
 }
 
 /* Select this frame.  */
diff --git a/gdb/testsuite/gdb.python/py-frame.c b/gdb/testsuite/gdb.python/py-frame.c
index 22eb9f2..82db341 100644
--- a/gdb/testsuite/gdb.python/py-frame.c
+++ b/gdb/testsuite/gdb.python/py-frame.c
@@ -8,7 +8,23 @@ int f1 (int a, int b)
   return f2(a) + b;
 }
 
+int block (void)
+{
+  int i = 99;
+  {
+    double i = 1.1;
+    double f = 2.2;
+    {
+      const char *i = "stuff";
+      const char *f = "foo";
+      const char *b = "bar";
+      return 0; /* Block break here.  */
+    }
+  }
+}
+
 int main (int argc, char *argv[])
 {
+  block ();
   return f1 (1, 2);
 }
diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
index 6989207..150e737 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -56,6 +56,30 @@ if ![runto_main] then {
     return 0
 }
 
+gdb_breakpoint [gdb_get_line_number "Block break here."]
+gdb_continue_to_breakpoint "Block break here."
+gdb_py_test_silent_cmd "python bf1 = gdb.selected_frame ()" "get frame" 0
+
+# First test that read_var is unaffected by PR 11036 changes.
+gdb_test "python print bf1.read_var(\"i\")" "\"stuff\"" "test i"
+gdb_test "python print bf1.read_var(\"f\")" "\"foo\"" "test f"
+gdb_test "python print bf1.read_var(\"b\")" "\"bar\"" "test b"
+
+# Test the read_var function in another block other than the current
+# block (in this case, the super block). Test thar read_var is reading
+# the correct variables of i and f but they are the correct value and type.
+gdb_py_test_silent_cmd "python sb = bf1.block().superblock" "get superblock" 0
+gdb_test "python print bf1.read_var(\"i\", sb)" "1.1.*" "test i = 1.1"
+gdb_test "python print bf1.read_var(\"i\", sb).type" "double" "test double i"
+gdb_test "python print bf1.read_var(\"f\", sb)" "2.2.*" "test f = 2.2"
+gdb_test "python print bf1.read_var(\"f\", sb).type" "double" "test double f"
+
+# And again test another outerblock, this time testing "i" is the
+# correct value and type.
+gdb_py_test_silent_cmd "python sb = sb.superblock" "get superblock" 0
+gdb_test "python print bf1.read_var(\"i\", sb)" "99" "test i = 99"
+gdb_test "python print bf1.read_var(\"i\", sb).type" "int" "test int i"
+
 gdb_breakpoint "f2"
 gdb_continue_to_breakpoint "breakpoint at f2"
 gdb_test "up" "" ""

Re: [python][patch] Fix 11036

[Eli Zaretskii]

> Date: Fri, 26 Feb 2010 12:37:50 +0000
> From: Phil Muldoon <pmuldoon@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> I have also updated the comments and the documentation to reflect the
> new decisions.

Thanks.  I have a couple of comments:

> +@defmethod Frame read_var variable @r{[}block@r{]}
> +Return the value of the given variable in this frame.  If the optional
> +block argument is provided, search for the variable from that current
> +block; otherwise start at the frame's current block (which is

Please use @var for the arguments:

  Return the value of @var{variable} in this frame.  If the optional
  argument @var{block} is provided, search for the variable from that
  block; otherwise start at the frame's current block (which is

(note that I also slightly reworded your text).

Okay with these changes.

Re: [python][patch] Fix 11036

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> If you examine the runtime, the return here is a valid value, not
Phil> a NULL.  In any case, I think that if read_var returns NULL, then this
Phil> is an error case.  If anyone knows this to be different let me know and
Phil> I can go back to returning None in this case.  To reflect this, if the
Phil> result of read_var is NULL, I raise an exception.  What do you think?

I think this is fine.
I dug through the code a bit and I also couldn't see when read_var_value
would return NULL.  Perhaps it is from before the LOC_COMPUTED case was
changed to set the "optimized out" flag?  (Just a random hypothesis
based on the documentation for symbol_computed_ops::read_variable.)

This patch is ok.

Tom

Re: [python][patch] Fix 11036

[Phil Muldoon]

On Fri, Feb 26, 2010 at 04:16:09PM -0700, Tom Tromey wrote:
> I think this is fine.

On Fri, Feb 26, 2010 at 02:55 PM, Eli Zaretskii wrote:
> (note that I also slightly reworded your text).
> 
> Okay with these changes.


So committed:

http://sourceware.org/ml/gdb-cvs/2010-02/msg00223.html

Cheers, and thanks for the help.

Phil Muldoon