From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31816 invoked by alias); 28 Jun 2011 22:14:55 -0000 Received: (qmail 31807 invoked by uid 22791); 28 Jun 2011 22:14:54 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Jun 2011 22:14:35 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p5SMEYvh026592 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 28 Jun 2011 18:14:34 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p5SMEXWC023053; Tue, 28 Jun 2011 18:14:33 -0400 From: Phil Muldoon To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [patch] [python] Implement Inferior.current_inferior References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Tue, 28 Jun 2011 22:14:00 -0000 In-Reply-To: (Tom Tromey's message of "Tue, 28 Jun 2011 14:29:52 -0600") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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 X-SW-Source: 2011-06/txt/msg00431.txt.bz2 Tom Tromey writes: >>>>>> "Phil" == Phil Muldoon writes: > > Phil> -/* Return a borrowed reference to the Python object of type Inferior > Phil> +/* Return a reference to the Python object of type Inferior > Phil> representing INFERIOR. If the object has already been created, > Phil> - return it, otherwise, create it. Return NULL on failure. */ > Phil> + return it and increment the reference count, otherwise, create it. > Phil> + Return NULL on failure. */ > Phil> PyObject * > Phil> inferior_to_inferior_object (struct inferior *inferior) > Phil> { > Phil> @@ -154,13 +155,14 @@ inferior_to_inferior_object (struct inferior *inferior) > > Phil> do_cleanups (cleanup); > Phil> } > Phil> + else > Phil> + Py_INCREF ((PyObject *)inf_obj); > > I don't really follow this. Well it took me a bit too. inferior_to_inferior_object is used in a multitude of scenarios. This patchlet addresses a new Python object created to follow an internal GDB inferior. Take this example: foo = gdb.current_inferior() bar = gdb.current_inferior() So the first time with foo, a new Python inferior object is created. And that reference is stashed in the GDB inferior object with set_inferior_data. The second time, inferior_to_inferior_object queries the inferior, finds the reference stashed in the GDB inferior and returns that stashed Python inferior object. But before this patch, bar would not have had the reference count incremented. This bug was not really exposed before the current_inferior patch. This patch fixes that. Why? Because, if you do: foo = None Then the reference count = 0, and bar points to garbage after Python GC's the foo object. > I think the best model would be that the 'struct inferior' owns a > reference to the gdb.Inferior object, and then > inferior_to_inferior_object is consistent: it either always returns a > new reference or a borrowed reference. In the context of this patch, I decided against a wholesale rewriting of how we store inferiors. Now that his patch is split, it might be time to implement your suggestion. > For a borrowed reference, I think you can leave this function as-is. > > For a new reference, I think you need to remove the 'else' from your > patch, and update infpy_dealloc to decref. Functions that return either a borrowed or new references really are bogus IMO. The consumer does not know what they are getting. inferior_to_inferior_object does this. This patch uniformly returns a new reference, and the callers are responsible for the life-cycle of that reference. While that means a little more busy-work for the callers, they can be consistent in dealing with the life-cycle of that object. That being said, I kind of like your idea above. Cheers, Phil