[PATCH] record_latest_value: Call release_value_or_incref instead of release_value

[Doug Evans <xdje42@gmail.com>]

Hi.

While testing a fix for bug 16612 I was getting segvs and traced it to
the new history-append! Scheme function.

The underlying gdb value was getting prematurely freed when its
Scheme wrapper was garbage collected, but it was still in the value
history.

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

This fix feels right but it could use another set of eyes.
I'm left wondering how much potential for future bugs there are
because other code is calling release_value when it should be calling
release_value_or_incref.
Often it doesn't matter because the value was just created and
after releasing the value there is only one reference.
However, ISTM this is quite fragile: I reviewed ludo's patch and I
didn't expect record_latest_value to get the reference counting wrong.

As a start, before making any more changes, I wonder if it would be
correct to add an assert to release_value to verify the value hasn't
already been released: If it has already been released chances are
the reference count will be wrong upon return.

Could be misunderstanding something of course.

Regression tested on amd64-linux.

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

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

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