[PATCH] record_latest_value: Call release_value_or_incref instead of release_value

[PATCH] record_latest_value: Call release_value_or_incref instead of release_value

[Doug Evans]

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

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

[Doug Evans]

On Sun, Mar 2, 2014 at 10:00 PM, Doug Evans <xdje42@gmail.com> wrote:
> 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.

Apologies for the followup.
I should have said "However it was one upon entry and the value had
already been released."
The value gets released when creating the Scheme wrapper.

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

[Ludovic Courtès]

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?

Thanks,
Ludo’.

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

[Joel Brobecker]

> --8<---------------cut here---------------start------------->8---
> (gdb) guile (use-modules (gdb))

Following the example of Python, shouldn't this be implicit?

> (gdb) guile (history-append! (make-value 42))

-- 
Joel

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

[Ludovic Courtès]

Joel Brobecker <brobecker@adacore.com> skribis:

>> --8<---------------cut here---------------start------------->8---
>> (gdb) guile (use-modules (gdb))
>
> Following the example of Python, shouldn't this be implicit?

Either way is fine with me.

I think Doug’s concern was about clashes, for instance because the (gdb)
module exports ‘symbol?’, which has nothing to do with Guile’s ‘symbol?’.

Ludo’.

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

[Doug Evans]

On Mon, Mar 3, 2014 at 5:23 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> --8<---------------cut here---------------start------------->8---
>> (gdb) guile (use-modules (gdb))
>
> Following the example of Python, shouldn't this be implicit?
>
>> (gdb) guile (history-append! (make-value 42))

We had a discussion of this on the #guile irc channel, and I couldn't
find anyone who thought it should be implicit.

Plus we also had a discussion of whether the symbols should have a
builtin prefix, e.g., to avoid collisions like the one with symbol?
that ludo mentions.
E.g., gdb:symbol? instead of symbol?

I couldn't find anyone who advocated for a builtin prefix, and Guile
provides the ability to add one when importing:
(use-modules ((gdb) #:renamer (symbol-prefix-proc 'gdb:)))

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

[Doug Evans]

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.

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

[Doug Evans]

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.