Re: [PATCH] record_latest_value: Call release_value_or_incref instead of release_value

[Doug Evans <xdje42 at gmail dot com>]

Doug Evans <xdje42@gmail.com> writes:

> On Mon, Mar 3, 2014 at 1:17 AM, Ludovic CourtÃs <ludo@gnu.org> wrote:
>> Doug Evans <xdje42@gmail.com> skribis:
>>
>>> I think this is because its reference counting is wrong.
>>> Upon return from record_latest_value, its reference count is still one.
>>> However it was one upon entry.  It should be two, right?
>>> One for the Scheme wrapper and one for the history entry.
>>>
>>> (gdb) guile (define histnum (history-append! (make-value 42)))
>>
>> Indeed, good catch.  Here's how I reproduced it:
>>
>> --8<---------------cut here---------------start------------->8---
>> (gdb) guile (use-modules (gdb))
>> (gdb) guile (history-append! (make-value 42))
>> 1
>> (gdb) p $1
>> $2 = 42
>> (gdb) guile (gc)
>> (gdb) p $1
>> Segmentation fault
>> --8<---------------cut here---------------end--------------->8---
>>
>> What about adding this to the patch as a test case?
>
> Shall do.  Thanks.

Here's what I committed.

2014-03-13  Doug Evans  <xdje42@gmail.com>

	* value.c (record_latest_value): Call release_value_or_incref
	instead of release_value.

testsuite/

2014-03-13  Ludovic CourtÃs  <ludo@gnu.org>
	    Doug Evans  <xdje42@gmail.com>

	* gdb.guile/scm-value.exp (test_value_in_inferior): Verify value added
	to history survives a gc.

diff --git a/gdb/value.c b/gdb/value.c
index 4e8d1fe..27043ee 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1659,7 +1659,11 @@ record_latest_value (struct value *val)
      from.  This is a bit dubious, because then *&$1 does not just return $1
      but the current contents of that location.  c'est la vie...  */
   val->modifiable = 0;
-  release_value (val);
+
+  /* The value may have already been released, in which case we're adding a
+     new reference for its entry in the history.  That is why we call
+     release_value_or_incref here instead of release_value.  */
+  release_value_or_incref (val);
 
   /* Here we treat value_history_count as origin-zero
      and applying to the value being stored now.  */


diff --git a/gdb/testsuite/gdb.guile/scm-value.exp b/gdb/testsuite/gdb.guile/scm-value.exp
index 89f0ff1..a85d5bd 100644
--- a/gdb/testsuite/gdb.guile/scm-value.exp
+++ b/gdb/testsuite/gdb.guile/scm-value.exp
@@ -67,6 +67,10 @@ proc test_value_in_inferior {} {
     gdb_test "gu (history-ref i)" "#<gdb:value 42>"
     gdb_test "p \$" "= 42"
 
+    # Verify the recorded history value survives a gc.
+    gdb_test_no_output "guile (gc)"
+    gdb_test "p \$\$" "= 42"
+
     # Test dereferencing the argv pointer.
 
     # Just get inferior variable argv the value history, available to guile.