Python: fetch value when building gdb.Value object

Python: fetch value when building gdb.Value object

[Paul Koning]

GDB sometimes lazily evaluates operations on values, and py-value.c wasn't taking that into account.  The result was that assigning a Value object to a Python variable could assign a lazy value, so that any errors in accessing the data would occur at a later time, and sometimes would not be handled right.  (For example, the "nonzero" operation would fail without a Python traceback.)

The attached patch cures this by fetching any lazy values when the gdb.Value object is built, and adds a test in the testcases to verify this.

Ok to submit?

	paul

ChangeLog:
	
2011-09-21  Paul Koning  <paul_koning@dell.com>

	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
	(value_to_value_object): Fetch value if it was lazy.

testsuite/ChangeLog:
	
2011-09-21  Paul Koning  <paul_koning@dell.com>

	* gdb.python/py-value.exp: Add test for null pointer reference
	assigned to a variable.

Index: python/py-value.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-value.c,v
retrieving revision 1.25
diff -u -r1.25 py-value.c
--- python/py-value.c	27 Jun 2011 19:21:51 -0000	1.25
+++ python/py-value.c	21 Sep 2011 15:45:12 -0000
@@ -209,7 +209,7 @@
 	val_obj->address = value_to_value_object (res_val);
     }
 
-  Py_INCREF (val_obj->address);
+  Py_XINCREF (val_obj->address);
 
   return val_obj->address;
 }
@@ -1045,7 +1045,15 @@
 value_to_value_object (struct value *val)
 {
   value_object *val_obj;
+  volatile struct gdb_exception except;
 
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      if (value_lazy (val))
+	value_fetch_lazy (val);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+  
   val_obj = PyObject_New (value_object, &value_object_type);
   if (val_obj != NULL)
     {
Index: testsuite/gdb.python/py-value.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.python/py-value.exp,v
retrieving revision 1.22
diff -u -r1.22 py-value.exp
--- testsuite/gdb.python/py-value.exp	26 Jul 2011 18:38:55 -0000	1.22
+++ testsuite/gdb.python/py-value.exp	21 Sep 2011 15:45:13 -0000
@@ -218,6 +218,7 @@
 
   # Test memory error.
   gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
+  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
 
   # Test string fetches,  both partial and whole.
   gdb_test "print st" "\"divide et impera\""

Re: Python: fetch value when building gdb.Value object

[Phil Muldoon]

Paul Koning <paulkoning@comcast.net> writes:

> GDB sometimes lazily evaluates operations on values, and py-value.c wasn't taking that into account.  The result was that assigning a Value object to a Python variable could assign a lazy value, so that any errors in accessing the data would occur at a later time, and sometimes would not be handled right.  (For example, the "nonzero" operation would fail without a Python traceback.)
>
> The attached patch cures this by fetching any lazy values when the gdb.Value object is built, and adds a test in the testcases to verify this.
>
> Ok to submit?
>
> 	paul
>
> ChangeLog:
> 	
> 2011-09-21  Paul Koning  <paul_koning@dell.com>
>
> 	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
> 	(value_to_value_object): Fetch value if it was lazy.
>
> testsuite/ChangeLog:
> 	
> 2011-09-21  Paul Koning  <paul_koning@dell.com>
>
> 	* gdb.python/py-value.exp: Add test for null pointer reference
> 	assigned to a variable.
>
> Index: python/py-value.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/python/py-value.c,v
> retrieving revision 1.25
> diff -u -r1.25 py-value.c
> --- python/py-value.c	27 Jun 2011 19:21:51 -0000	1.25
> +++ python/py-value.c	21 Sep 2011 15:45:12 -0000
> @@ -209,7 +209,7 @@
>  	val_obj->address = value_to_value_object (res_val);
>      }
>  
> -  Py_INCREF (val_obj->address);
> +  Py_XINCREF (val_obj->address);
>  
>    return val_obj->address;
>  }

This seems an unrelated change?

> @@ -1045,7 +1045,15 @@
>  value_to_value_object (struct value *val)
>  {
>    value_object *val_obj;
> +  volatile struct gdb_exception except;
>  
> +  TRY_CATCH (except, RETURN_MASK_ALL)
> +    {

Something that Jan pointed out a few weeks ago, is our exception net is
too wide, and asked me to review usage of REVIEW_MASK_ALL.  In this
case, this should probably be RETURN_MASK_ERROR.  I understand there are
many many usages of RETURN_MASK_ALL used incorrectly already.


>    # Test memory error.
>    gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
> +  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"

What scenario will this test catch that the previous test won't?  I'm
not saying you are incorrect, I just don't understand. What
error-trigger does the assignment to "inval" trigger?

Cheers,

Phil

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Sep 28, 2011, at 3:24 PM, Phil Muldoon wrote:

> Paul Koning <paulkoning@comcast.net> writes:
> 
>> GDB sometimes lazily evaluates operations on values, and py-value.c wasn't taking that into account.  The result was that assigning a Value object to a Python variable could assign a lazy value, so that any errors in accessing the data would occur at a later time, and sometimes would not be handled right.  (For example, the "nonzero" operation would fail without a Python traceback.)
>> 
>> The attached patch cures this by fetching any lazy values when the gdb.Value object is built, and adds a test in the testcases to verify this.
>> 
>> Ok to submit?
>> 
>> 	paul
>> 
>> ChangeLog:
>> 	
>> 2011-09-21  Paul Koning  <paul_koning@dell.com>
>> 
>> 	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
>> 	(value_to_value_object): Fetch value if it was lazy.
>> 
>> testsuite/ChangeLog:
>> 	
>> 2011-09-21  Paul Koning  <paul_koning@dell.com>
>> 
>> 	* gdb.python/py-value.exp: Add test for null pointer reference
>> 	assigned to a variable.
>> 
>> Index: python/py-value.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/python/py-value.c,v
>> retrieving revision 1.25
>> diff -u -r1.25 py-value.c
>> --- python/py-value.c	27 Jun 2011 19:21:51 -0000	1.25
>> +++ python/py-value.c	21 Sep 2011 15:45:12 -0000
>> @@ -209,7 +209,7 @@
>> 	val_obj->address = value_to_value_object (res_val);
>>     }
>> 
>> -  Py_INCREF (val_obj->address);
>> +  Py_XINCREF (val_obj->address);
>> 
>>   return val_obj->address;
>> }
> 
> This seems an unrelated change?

With the next change, value_to_value_object can return NULL (if the value_fetch_lazy generates an error).  So we now need XINCREF since that works with a null pointer, while INCREF will crash if handed a null pointer.  This is standard Python API coding style when dealing with a pointer that might be null because of an earlier failure, or because of a code path that doesn't set it.

> 
>> @@ -1045,7 +1045,15 @@
>> value_to_value_object (struct value *val)
>> {
>>   value_object *val_obj;
>> +  volatile struct gdb_exception except;
>> 
>> +  TRY_CATCH (except, RETURN_MASK_ALL)
>> +    {
> 
> Something that Jan pointed out a few weeks ago, is our exception net is
> too wide, and asked me to review usage of REVIEW_MASK_ALL.  In this
> case, this should probably be RETURN_MASK_ERROR.  

I'll investigate.  I'm not familiar with the GDB error catching machinery so I have some learning to do.  No problem.

> 
> 
>>   # Test memory error.
>>   gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
>> +  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
> 
> What scenario will this test catch that the previous test won't?  I'm
> not saying you are incorrect, I just don't understand. What
> error-trigger does the assignment to "inval" trigger?

If you execute that statement on the existing code, the statement will succeed with no error reported.  However, when you then attempt to use the value, you get an error at that time:

(gdb) python inval = gdb.parse_and_eval('*(int*)0')
(gdb) python foo=inval+1
Traceback (most recent call last):
  File "<string>", line 1, in ?
gdb.MemoryError: Cannot access memory at address 0x0
Error while executing Python code.
(gdb) python bar=bool(inval)
Cannot access memory at address 0x0
(gdb) quit

The first of those two has a traceback because the gdb.Value arithmetic operations have a TRY_CATCH in them.  The second one has no traceback because the "nonzero" method of gdb.Value doesn't do TRY_CATCH.  

With the patch you get this:

(gdb) python inval = gdb.parse_and_eval('*(int*)0')
Traceback (most recent call last):
  File "<string>", line 1, in ?
gdb.MemoryError: Cannot access memory at address 0x0
Error while executing Python code.

I.e., you get the error at the time you write the operation that performs the bad access.  The reason the previous test (with the print) works as expected is that it creates the gdb.Value object (which would work with no error) and then asks for the value of that object to print it (that's when the error is noticed).  With the patch it still works the same way as far as the output you see, although now the error is detected earlier in the execution of that statement.

	paul

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Sep 28, 2011, at 3:24 PM, Phil Muldoon wrote:

> Paul Koning <paulkoning@comcast.net> writes:
> 
>> ...
>> Index: python/py-value.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/python/py-value.c,v
>> retrieving revision 1.25
>> diff -u -r1.25 py-value.c
>> --- python/py-value.c	27 Jun 2011 19:21:51 -0000	1.25
>> +++ python/py-value.c	21 Sep 2011 15:45:12 -0000
>> ...
> 
>> @@ -1045,7 +1045,15 @@
>> value_to_value_object (struct value *val)
>> {
>>   value_object *val_obj;
>> +  volatile struct gdb_exception except;
>> 
>> +  TRY_CATCH (except, RETURN_MASK_ALL)
>> +    {
> 
> Something that Jan pointed out a few weeks ago, is our exception net is
> too wide, and asked me to review usage of REVIEW_MASK_ALL.  In this
> case, this should probably be RETURN_MASK_ERROR. 

Question on that...

If I use RETURN_MASK_ERROR and control/C is hit, does that mean the currently running code aborts and the outer handler that does have RETURN_MASK_ALL is entered?

If so, most of the Python code seems to be a candidate for RETURN_MASK_ERROR.  One exception is valpy_getitem (in py-value.c) since it has an xfree(value) after the TRY_CATCH but before the GDB_PY_HANDLE_EXCEPTION (outside the TRY_CATCH block, though -- is that right?)

Probably related: I just tried an infinite Python loop and found that control-C had no effect.  I wonder if the Python interpreter is setting up its own control-C trap (quite possibly -- that's a Python exception after all) and we're losing it somewhere along the lines.

	paul

[PING] [RFA] Re: Python: fetch value when building gdb.Value object

[Paul Koning]

Ping...

http://sourceware.org/ml/gdb-patches/2011-09/msg00390.html

	paul

Re: Python: fetch value when building gdb.Value object

[Jan Kratochvil]

On Wed, 28 Sep 2011 22:40:50 +0200, Paul Koning wrote:
> If I use RETURN_MASK_ERROR and control/C is hit, does that mean the
> currently running code aborts and the outer handler that does have
> RETURN_MASK_ALL is entered?

Yes.

> If so, most of the Python code seems to be a candidate for
> RETURN_MASK_ERROR.

In fact I do not know, it is a Python thing.

RETURN_MASK_ALL is right if returned PyExc_KeyboardInterrupt will really abort
any execution of Python code.  It is probably so, as suggested by:
	http://docs.python.org/library/exceptions.html#exceptions.KeyboardInterrupt

RETURN_MASK_ERROR is right otherwise, but only if it is safe to longjmp out
from a code called by Python.  This may not be true.  Python may be C++
exceptions throwing safe but it cannot be safe for the GDB longjmp exceptions.
But this case would mean Python is buggy for CTRL-C on its own so
RETURN_MASK_ERROR probably is not right.


> One exception is valpy_getitem (in py-value.c) since it has an xfree(value)
> after the TRY_CATCH but before the GDB_PY_HANDLE_EXCEPTION (outside the
> TRY_CATCH block, though -- is that right?)

It is right as long as RETURN_MASK_ALL is right.  It is more usual to call
make_cleanup though - as shown in the attached change.  The change could be
just a code cleanup without functionality change if that RETURN_MASK_ALL is
kept there.


> Probably related: I just tried an infinite Python loop and found that
> control-C had no effect.  I wonder if the Python interpreter is setting up
> its own control-C trap (quite possibly -- that's a Python exception after
> all) and we're losing it somewhere along the lines.

Yes, this is the kind of bug from it, thanks for checking it.


Thanks,
Jan


only FYI, not intended for commit:

gdb/
2011-10-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* python/py-value.c (valpy_getitem): New variable back_to.  Register
	xfree of field to it.  Call do_cleanups for it.  Use RETURN_MASK_ERROR
	instead of RETURN_MASK_ALL.

--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -448,6 +448,7 @@ valpy_getitem (PyObject *self, PyObject *key)
   char *field = NULL;
   struct value *res_val = NULL;
   volatile struct gdb_exception except;
+  struct cleanup *back_to;
 
   if (gdbpy_is_string (key))
     {  
@@ -456,7 +457,9 @@ valpy_getitem (PyObject *self, PyObject *key)
 	return NULL;
     }
 
-  TRY_CATCH (except, RETURN_MASK_ALL)
+  back_to = make_cleanup (xfree, field);
+
+  TRY_CATCH (except, RETURN_MASK_ERROR)
     {
       struct value *tmp = self_value->value;
 
@@ -485,10 +488,10 @@ valpy_getitem (PyObject *self, PyObject *key)
 	    }
 	}
     }
-
-  xfree (field);
   GDB_PY_HANDLE_EXCEPTION (except);
 
+  do_cleanups (back_to);
+
   return res_val ? value_to_value_object (res_val) : NULL;
 }
 

Re: Python: fetch value when building gdb.Value object

[Jan Kratochvil]

On Wed, 28 Sep 2011 21:24:45 +0200, Phil Muldoon wrote:
> What scenario will this test catch that the previous test won't?  I'm
> not saying you are incorrect, I just don't understand. What
> error-trigger does the assignment to "inval" trigger?

I would prefer here a testcase more clearly showing the bug, attached below.

I believe the patch is right, as Phil hasn't yet agreed posting it only.


Thanks,
Jan


gdb/
2011-09-21  Paul Koning  <paul_koning@dell.com>

	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
	(value_to_value_object): Fetch value if it was lazy.

testsuite/
2011-09-21  Paul Koning  <paul_koning@dell.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.python/py-value.exp
	(python inval = gdb.parse_and_eval('*(int*)0'))
	(python argc_lazy = gdb.parse_and_eval('argc'), sanity check argc)
	(set argc=2, python print argc_lazy): New tests.

--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -209,7 +209,7 @@ valpy_get_address (PyObject *self, void *closure)
 	val_obj->address = value_to_value_object (res_val);
     }
 
-  Py_INCREF (val_obj->address);
+  Py_XINCREF (val_obj->address);
 
   return val_obj->address;
 }
@@ -1045,7 +1045,15 @@ PyObject *
 value_to_value_object (struct value *val)
 {
   value_object *val_obj;
+  volatile struct gdb_exception except;
 
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      if (value_lazy (val))
+	value_fetch_lazy (val);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+  
   val_obj = PyObject_New (value_object, &value_object_type);
   if (val_obj != NULL)
     {
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -218,6 +218,11 @@ proc test_value_in_inferior {} {
 
   # Test memory error.
   gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
+  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
+  gdb_test "python argc_lazy = gdb.parse_and_eval('argc')"
+  gdb_test "print argc" " = 1" "sanity check argc"
+  gdb_test_no_output "set argc=2"
+  gdb_test "python print argc_lazy" "\r\n1"
 
   # Test string fetches,  both partial and whole.
   gdb_test "print st" "\"divide et impera\""

Re: Python: fetch value when building gdb.Value object

[Pedro Alves]

On Saturday 01 October 2011 10:28:52, Jan Kratochvil wrote:
>    # Test memory error.
>    gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
> +  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"

I see this is already the case, but just a reminder that this will fail
(or rather the write will succeed) on targets without an MMU.  We should
skip it with something like:

set can_read_zero 0
gdb_test_multiple "x 0" "memory at address 0" {
    -re "0x0:.*Cannot access memory at address 0x0.*$gdb_prompt $" { }
    -re "0x0:.*Error accessing memory address 0x0.*$gdb_prompt $" { }
    -re ".*$gdb_prompt $" {
        set can_read_zero 1
    }
}

if { !$can_read_zero } {
  gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
}

-- 
Pedro Alves

Re: Python: fetch value when building gdb.Value object

[Jan Kratochvil]

On Sat, 01 Oct 2011 12:22:46 +0200, Pedro Alves wrote:
> On Saturday 01 October 2011 10:28:52, Jan Kratochvil wrote:
> >    # Test memory error.
> >    gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
> > +  gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
> 
> I see this is already the case, but just a reminder that this will fail
> (or rather the write will succeed) on targets without an MMU.  We should
> skip it with something like:

So checked it in (I am aware the existing code elsewhere in GDB testsuite is
a bit different).


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-10/msg00002.html

--- src/gdb/testsuite/ChangeLog	2011/09/30 15:07:32	1.2867
+++ src/gdb/testsuite/ChangeLog	2011/10/01 11:02:10	1.2868
@@ -1,3 +1,12 @@
+2011-10-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Pedro Alves  <pedro@codesourcery.com>
+
+	* gdb.python/py-value.exp (test_value_in_inferior): New variable
+	can_read_0, test for it.
+	(python print gdb.parse_and_eval('*(int*)0')): Rename to ...
+	(parse_and_eval with memory error): ... here, make it untested if
+	can_read_0.
+
 2011-09-30  Marek Polacek  <mpolacek@redhat.com>
 
 	* gdb.python/python.exp (verify pagination beforehand)
--- src/gdb/testsuite/gdb.python/py-value.exp	2011/07/26 18:38:55	1.22
+++ src/gdb/testsuite/gdb.python/py-value.exp	2011/10/01 11:02:11	1.23
@@ -216,8 +216,25 @@
   # Test address attribute
   gdb_test "python print 'result =', arg0.address" "= 0x\[\[:xdigit:\]\]+" "Test address attribute"
 
+  # Test displaying a variable that is temporarily at a bad address.
+  # But if we can examine what's at memory address 0, then we'll also be
+  # able to display it without error.  Don't run the test in that case.
+  set can_read_0 0
+  gdb_test_multiple "x 0" "memory at address 0" {
+      -re "0x0:\[ \t\]*Cannot access memory at address 0x0\r\n$gdb_prompt $" { }
+      -re "0x0:\[ \t\]*Error accessing memory address 0x0\r\n$gdb_prompt $" { }
+      -re "\r\n$gdb_prompt $" {
+	  set can_read_0 1
+      }
+  }
+
   # Test memory error.
-  gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*"
+  set test "parse_and_eval with memory error"
+  if {$can_read_0} {
+    untested $test
+  } else {
+    gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*" $test
+  }
 
   # Test string fetches,  both partial and whole.
   gdb_test "print st" "\"divide et impera\""

Re: Python: fetch value when building gdb.Value object [rediff]

[Jan Kratochvil]

Updated for current HEAD:

On Sat, 01 Oct 2011 11:28:52 +0200, Jan Kratochvil wrote:
On Wed, 28 Sep 2011 21:24:45 +0200, Phil Muldoon wrote:
> What scenario will this test catch that the previous test won't?  I'm
> not saying you are incorrect, I just don't understand. What
> error-trigger does the assignment to "inval" trigger?

I would prefer here a testcase more clearly showing the bug, attached below.

I believe the patch is right, as Phil hasn't yet agreed posting it only.


Thanks,
Jan


gdb/
2011-09-21  Paul Koning  <paul_koning@dell.com>

	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
	(value_to_value_object): Fetch value if it was lazy.

testsuite/
2011-09-21  Paul Koning  <paul_koning@dell.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.python/py-value.exp
	(python inval = gdb.parse_and_eval('*(int*)0'))
	(python argc_lazy = gdb.parse_and_eval('argc'), sanity check argc)
	(set argc=2, python print argc_lazy): New tests.

--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -209,7 +209,7 @@ valpy_get_address (PyObject *self, void *closure)
 	val_obj->address = value_to_value_object (res_val);
     }
 
-  Py_INCREF (val_obj->address);
+  Py_XINCREF (val_obj->address);
 
   return val_obj->address;
 }
@@ -1045,7 +1045,15 @@ PyObject *
 value_to_value_object (struct value *val)
 {
   value_object *val_obj;
+  volatile struct gdb_exception except;
 
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      if (value_lazy (val))
+	value_fetch_lazy (val);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+  
   val_obj = PyObject_New (value_object, &value_object_type);
   if (val_obj != NULL)
     {
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -236,6 +236,18 @@ proc test_value_in_inferior {} {
     gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*" $test
   }
 
+  # Test Python values are not lazy.
+  set test "memory error occurs even for possibly lazy values"
+  if {$can_read_0} {
+    untested $test
+  } else {
+    gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*" $test
+  }
+  gdb_test "python argc_lazy = gdb.parse_and_eval('argc')"
+  gdb_test "print argc" " = 1" "sanity check argc"
+  gdb_test_no_output "set argc=2"
+  gdb_test "python print argc_lazy" "\r\n1"
+
   # Test string fetches,  both partial and whole.
   gdb_test "print st" "\"divide et impera\""
   gdb_py_test_silent_cmd "python st = gdb.history (0)" "get value from history" 1

Re: Python: fetch value when building gdb.Value object [rediff]

[Paul Koning]

On Oct 1, 2011, at 8:16 AM, Jan Kratochvil wrote:

> Updated for current HEAD:
> 
> On Sat, 01 Oct 2011 11:28:52 +0200, Jan Kratochvil wrote:
> On Wed, 28 Sep 2011 21:24:45 +0200, Phil Muldoon wrote:
>> What scenario will this test catch that the previous test won't?  I'm
>> not saying you are incorrect, I just don't understand. What
>> error-trigger does the assignment to "inval" trigger?
> 
> I would prefer here a testcase more clearly showing the bug, attached below.

Thanks, yes I agree that's a clearer and more complete testcase.

	paul

> 
> I believe the patch is right, as Phil hasn't yet agreed posting it only.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2011-09-21  Paul Koning  <paul_koning@dell.com>
> 
> 	* python/py-value.c (valpy_get_address): Use Py_XINCREF.
> 	(value_to_value_object): Fetch value if it was lazy.
> 
> testsuite/
> 2011-09-21  Paul Koning  <paul_koning@dell.com>
> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.python/py-value.exp
> 	(python inval = gdb.parse_and_eval('*(int*)0'))
> 	(python argc_lazy = gdb.parse_and_eval('argc'), sanity check argc)
> 	(set argc=2, python print argc_lazy): New tests.
> 
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -209,7 +209,7 @@ valpy_get_address (PyObject *self, void *closure)
> 	val_obj->address = value_to_value_object (res_val);
>     }
> 
> -  Py_INCREF (val_obj->address);
> +  Py_XINCREF (val_obj->address);
> 
>   return val_obj->address;
> }
> @@ -1045,7 +1045,15 @@ PyObject *
> value_to_value_object (struct value *val)
> {
>   value_object *val_obj;
> +  volatile struct gdb_exception except;
> 
> +  TRY_CATCH (except, RETURN_MASK_ALL)
> +    {
> +      if (value_lazy (val))
> +	value_fetch_lazy (val);
> +    }
> +  GDB_PY_HANDLE_EXCEPTION (except);
> +  
>   val_obj = PyObject_New (value_object, &value_object_type);
>   if (val_obj != NULL)
>     {
> --- a/gdb/testsuite/gdb.python/py-value.exp
> +++ b/gdb/testsuite/gdb.python/py-value.exp
> @@ -236,6 +236,18 @@ proc test_value_in_inferior {} {
>     gdb_test "python print gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*" $test
>   }
> 
> +  # Test Python values are not lazy.
> +  set test "memory error occurs even for possibly lazy values"
> +  if {$can_read_0} {
> +    untested $test
> +  } else {
> +    gdb_test "python inval = gdb.parse_and_eval('*(int*)0')" "gdb.MemoryError: Cannot access memory at address 0x0.*" $test
> +  }
> +  gdb_test "python argc_lazy = gdb.parse_and_eval('argc')"
> +  gdb_test "print argc" " = 1" "sanity check argc"
> +  gdb_test_no_output "set argc=2"
> +  gdb_test "python print argc_lazy" "\r\n1"
> +
>   # Test string fetches,  both partial and whole.
>   gdb_test "print st" "\"divide et impera\""
>   gdb_py_test_silent_cmd "python st = gdb.history (0)" "get value from history" 1

Re: Python: fetch value when building gdb.Value object [rediff]

[Phil Muldoon]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> Updated for current HEAD:
>
> On Sat, 01 Oct 2011 11:28:52 +0200, Jan Kratochvil wrote:
> On Wed, 28 Sep 2011 21:24:45 +0200, Phil Muldoon wrote:
>> What scenario will this test catch that the previous test won't?  I'm
>> not saying you are incorrect, I just don't understand. What
>> error-trigger does the assignment to "inval" trigger?
>
> I would prefer here a testcase more clearly showing the bug, attached below.
>
> I believe the patch is right, as Phil hasn't yet agreed posting it only.

It looks great, thanks for doing this.

Cheers,

Phil

Re: Python: fetch value when building gdb.Value object

[Phil Muldoon]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> On Wed, 28 Sep 2011 22:40:50 +0200, Paul Koning wrote:
>> If I use RETURN_MASK_ERROR and control/C is hit, does that mean the
>> currently running code aborts and the outer handler that does have
>> RETURN_MASK_ALL is entered?
>
> Yes.
>
>> If so, most of the Python code seems to be a candidate for
>> RETURN_MASK_ERROR.
>
> In fact I do not know, it is a Python thing.
>
> RETURN_MASK_ALL is right if returned PyExc_KeyboardInterrupt will really abort
> any execution of Python code.  It is probably so, as suggested by:
> 	http://docs.python.org/library/exceptions.html#exceptions.KeyboardInterrupt
>
> RETURN_MASK_ERROR is right otherwise, but only if it is safe to longjmp out
> from a code called by Python.  This may not be true.  Python may be C++
> exceptions throwing safe but it cannot be safe for the GDB longjmp exceptions.
> But this case would mean Python is buggy for CTRL-C on its own so
> RETURN_MASK_ERROR probably is not right.

Jan asked me to look at all the cases.  I just have not had time to do
it yet.  Too me, the RETURN_MASK in many cases in the Python code is far
too liberal, and should, as Jan notes, be reduced to RETURN_MASK_ERROR.
I am not sure it is just a mechanical change as each scenario has to be
carefully reviewed to understand if in fact an interrupt should be
allowed according to the Python API.

Cheers,

Phil

Re: Python: fetch value when building gdb.Value object [rediff]

[Paul Koning]

On Oct 3, 2011, at 6:18 AM, Phil Muldoon wrote:

> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
>> Updated for current HEAD:
>> 
>> On Sat, 01 Oct 2011 11:28:52 +0200, Jan Kratochvil wrote:
>> On Wed, 28 Sep 2011 21:24:45 +0200, Phil Muldoon wrote:
>>> What scenario will this test catch that the previous test won't?  I'm
>>> not saying you are incorrect, I just don't understand. What
>>> error-trigger does the assignment to "inval" trigger?
>> 
>> I would prefer here a testcase more clearly showing the bug, attached below.
>> 
>> I believe the patch is right, as Phil hasn't yet agreed posting it only.
> 
> It looks great, thanks for doing this.

Thanks all.

Committed.

	paul

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Paul> If so, most of the Python code seems to be a candidate for
Paul> RETURN_MASK_ERROR.

Jan> In fact I do not know, it is a Python thing.
Jan> RETURN_MASK_ALL is right if returned PyExc_KeyboardInterrupt will really abort
Jan> any execution of Python code.  It is probably so, as suggested by:
Jan> 	http://docs.python.org/library/exceptions.html#exceptions.KeyboardInterrupt

Jan> RETURN_MASK_ERROR is right otherwise, but only if it is safe to
Jan> longjmp out from a code called by Python.  This may not be true.
Jan> Python may be C++ exceptions throwing safe but it cannot be safe
Jan> for the GDB longjmp exceptions.  But this case would mean Python is
Jan> buggy for CTRL-C on its own so RETURN_MASK_ERROR probably is not
Jan> right.

I think the Python code in GDB should use RETURN_MASK_ALL in all cases.
The C-c case should be turned into PyExc_KeyboardInterrupt.
Other exceptions should either be ignored or turned into the appropriate
Python exception, depending on the situation.

Letting the C-c case longjmp over the Python implementation is bad, I
would imagine it will result in crashes.  Ignoring it is also bad.

Paul> Probably related: I just tried an infinite Python loop and found
Paul> that control-C had no effect.  I wonder if the Python interpreter
Paul> is setting up its own control-C trap (quite possibly -- that's a
Paul> Python exception after all) and we're losing it somewhere along
Paul> the lines.

Jan> Yes, this is the kind of bug from it, thanks for checking it.

I suspect we have to override Python's SIGINT handling.
I haven't investigated.

Jan> 2011-10-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* python/py-value.c (valpy_getitem): New variable back_to.  Register
Jan> 	xfree of field to it.  Call do_cleanups for it.  Use RETURN_MASK_ERROR
Jan> 	instead of RETURN_MASK_ALL.

This patch runs the cleanups too late.  They have to be done before
GDB_PY_HANDLE_EXCEPTION, as that has an early return in it.  Cleanest is
probably doing it all inside the TRY_CATCH.

Tom

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Paul> The first of those two has a traceback because the gdb.Value
Paul> arithmetic operations have a TRY_CATCH in them.  The second one
Paul> has no traceback because the "nonzero" method of gdb.Value doesn't
Paul> do TRY_CATCH.

It is a bug that nonzero doesn't use TRY_CATCH.

In the Python-facing code, the rule is that any calls into gdb proper
which may throw must be wrapped in TRY_CATCH and then must have
exception-conversion code, usually just GDB_PY_HANDLE_EXCEPTION.

This rule is needed to interface properly between gdb exceptions (which
longjmp) and Python exceptions (which do not).  Omitting it can lead, I
believe, to crashes.

Tom

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Paul> GDB sometimes lazily evaluates operations on values, and
Paul> py-value.c wasn't taking that into account.  The result was that
Paul> assigning a Value object to a Python variable could assign a lazy
Paul> value, so that any errors in accessing the data would occur at a
Paul> later time, and sometimes would not be handled right.  (For
Paul> example, the "nonzero" operation would fail without a Python
Paul> traceback.)  The attached patch cures this by fetching any lazy
Paul> values when the gdb.Value object is built, and adds a test in the
Paul> testcases to verify this.

Paul> Ok to submit?

I am not convinced that this is the right approach.

I think it would probably be better to expose the laziness to the Python
programmer -- via a new attribute and a new method to un-lazy the
object.

The reason is that eager fetching can be very expensive.  E.g., you may
construct an intermediate value that is a very large array, but intend
only to reference a few elements.  This can be done efficiently by gdb,
but eager fetching will defeat that.

Tom

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Oct 4, 2011, at 11:44 AM, Tom Tromey wrote:

>>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:
> 
> Paul> GDB sometimes lazily evaluates operations on values, and
> Paul> py-value.c wasn't taking that into account.  The result was that
> Paul> assigning a Value object to a Python variable could assign a lazy
> Paul> value, so that any errors in accessing the data would occur at a
> Paul> later time, and sometimes would not be handled right.  (For
> Paul> example, the "nonzero" operation would fail without a Python
> Paul> traceback.)  The attached patch cures this by fetching any lazy
> Paul> values when the gdb.Value object is built, and adds a test in the
> Paul> testcases to verify this.
> 
> Paul> Ok to submit?
> 
> I am not convinced that this is the right approach.
> 
> I think it would probably be better to expose the laziness to the Python
> programmer -- via a new attribute and a new method to un-lazy the
> object.
> 
> The reason is that eager fetching can be very expensive.  E.g., you may
> construct an intermediate value that is a very large array, but intend
> only to reference a few elements.  This can be done efficiently by gdb,
> but eager fetching will defeat that.

I modeled what I did after the way the existing GDB code handles convenience variables.  It seemed logical that

	set $foo = *ptr

and
	python foo=gdb.eval ("*ptr")

should behave the same.  With the patch, they do, because in both case the lazy evaluation is done. 

	paul

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Tom> I am not convinced that this is the right approach.

Tom> I think it would probably be better to expose the laziness to the Python
Tom> programmer -- via a new attribute and a new method to un-lazy the
Tom> object.

Tom> The reason is that eager fetching can be very expensive.  E.g., you may
Tom> construct an intermediate value that is a very large array, but intend
Tom> only to reference a few elements.  This can be done efficiently by gdb,
Tom> but eager fetching will defeat that.

Paul> I modeled what I did after the way the existing GDB code handles
Paul> convenience variables.

Convenience variables aren't really the best model.  First, they are
very old, probably predating anybody worrying about this stuff.  Second,
the CLI isn't really a fully-featured programming language.

Tom

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Oct 14, 2011, at 4:03 PM, Tom Tromey wrote:

>>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:
> 
> Tom> I am not convinced that this is the right approach.
> 
> Tom> I think it would probably be better to expose the laziness to the Python
> Tom> programmer -- via a new attribute and a new method to un-lazy the
> Tom> object.
> 
> Tom> The reason is that eager fetching can be very expensive.  E.g., you may
> Tom> construct an intermediate value that is a very large array, but intend
> Tom> only to reference a few elements.  This can be done efficiently by gdb,
> Tom> but eager fetching will defeat that.
> 
> Paul> I modeled what I did after the way the existing GDB code handles
> Paul> convenience variables.
> 
> Convenience variables aren't really the best model.  First, they are
> very old, probably predating anybody worrying about this stuff.  Second,
> the CLI isn't really a fully-featured programming language.

That's fair.

At this point the change has been approved and committed (that happened before your comments arrived).  So I guess we have a question of whether to change it back.

	paul

Re: Python: fetch value when building gdb.Value object

[Jim Blandy]

On Tue, Oct 4, 2011 at 8:44 AM, Tom Tromey <tromey@redhat.com> wrote:
> The reason is that eager fetching can be very expensive.  E.g., you may
> construct an intermediate value that is a very large array, but intend
> only to reference a few elements.  This can be done efficiently by gdb,
> but eager fetching will defeat that.

Yeah, this worried me, too. In C, array values are automatically
converted to pointers to their first element, but GDB doesn't (last I
checked) do this; instead, it relies on the lazy value fetching to get
the same effect. Unfortunately, this means that unlazying values can
have drastic effects on memory consumption that one would not expect
if one knows the C semantics.

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Paul> At this point the change has been approved and committed (that
Paul> happened before your comments arrived).  So I guess we have a question
Paul> of whether to change it back.

Yes, I think so.  I think the current approach is going to bite us
later.  We already have bugs open about bad memory management using
gdb.Value.

Tom

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Oct 19, 2011, at 4:51 PM, Tom Tromey wrote:

>>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:
> 
> Paul> At this point the change has been approved and committed (that
> Paul> happened before your comments arrived).  So I guess we have a question
> Paul> of whether to change it back.
> 
> Yes, I think so.  I think the current approach is going to bite us
> later.  We already have bugs open about bad memory management using
> gdb.Value.

Ok, I will work on that.

I think the exception handling fixes that were done recently takes care of the fact that some of the places where the lazy fetch exception could occur weren't correctly trapped inside Python.  Then the other part will be a documentation change, to spell out clearly the fact that operating on a gdb.Value type can raise an exception at that time.  For example, for a Value object v, the expression "v+1" can result in an exception (unlike what is usually true for Python variables of most types).

	paul

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

Tom> Yes, I think so.  I think the current approach is going to bite us
Tom> later.  We already have bugs open about bad memory management using
Tom> gdb.Value.

Paul> Ok, I will work on that.

Thanks.

Paul> Then the other part will be a documentation change, to spell out
Paul> clearly the fact that operating on a gdb.Value type can raise an
Paul> exception at that time.  For example, for a Value object v, the
Paul> expression "v+1" can result in an exception (unlike what is
Paul> usually true for Python variables of most types).

Yes, that sounds good.

Also, we can expose the laziness directly to Python.  I filed a PR for
this:

http://sourceware.org/bugzilla/show_bug.cgi?id=13327

Tom

Re: Python: fetch value when building gdb.Value object

[Paul Koning]

On Oct 20, 2011, at 11:03 AM, Tom Tromey wrote:

> Tom> Yes, I think so.  I think the current approach is going to bite us
> Tom> later.  We already have bugs open about bad memory management using
> Tom> gdb.Value.
> 
> Paul> Ok, I will work on that.
> 
> Thanks.
> 
> Paul> Then the other part will be a documentation change, to spell out
> Paul> clearly the fact that operating on a gdb.Value type can raise an
> Paul> exception at that time.  For example, for a Value object v, the
> Paul> expression "v+1" can result in an exception (unlike what is
> Paul> usually true for Python variables of most types).
> 
> Yes, that sounds good.
> 
> Also, we can expose the laziness directly to Python.  I filed a PR for
> this:
> 
> http://sourceware.org/bugzilla/show_bug.cgi?id=13327

I can make that part of the change.

How about an attribute "is_lazy", which reads as True or False, and can be set to False (but not to other values)?

	paul

Re: Python: fetch value when building gdb.Value object

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Paul> I can make that part of the change.

Paul> How about an attribute "is_lazy", which reads as True or False, and
Paul> can be set to False (but not to other values)?

I think for un-lazying it would be better to have an explicit method.
Setting an attribute to False and then having a side-effect seems too
obscure to me.

Tom