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

[Paul Koning <paulkoning@comcast.net>]

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