From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29545 invoked by alias); 3 Mar 2014 06:00:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29498 invoked by uid 89); 3 Mar 2014 06:00:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pd0-f179.google.com Received: from mail-pd0-f179.google.com (HELO mail-pd0-f179.google.com) (209.85.192.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 03 Mar 2014 06:00:31 +0000 Received: by mail-pd0-f179.google.com with SMTP id w10so3295320pde.38 for ; Sun, 02 Mar 2014 22:00:28 -0800 (PST) X-Received: by 10.68.249.100 with SMTP id yt4mr1124410pbc.165.1393826428518; Sun, 02 Mar 2014 22:00:28 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id kc9sm31891824pbc.25.2014.03.02.22.00.26 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 02 Mar 2014 22:00:27 -0800 (PST) From: Doug Evans To: gdb-patches@sourceware.org, ludo@gnu.org Subject: [PATCH] record_latest_value: Call release_value_or_incref instead of release_value Date: Mon, 03 Mar 2014 06:00:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-03/txt/msg00028.txt.bz2 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 * 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. */