From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52980 invoked by alias); 3 Aug 2016 11:35: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 52963 invoked by uid 89); 3 Aug 2016 11:35:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=leaks X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 03 Aug 2016 11:35:34 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B0B13B713; Wed, 3 Aug 2016 11:35:33 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u73BZVJE001893; Wed, 3 Aug 2016 07:35:32 -0400 Subject: Re: [RFA] PR python/18565 - make Frame.function work for inline frames To: Yao Qi References: <1466439050-11330-1-git-send-email-tom@tromey.com> <86ziqfq6sz.fsf@gmail.com> <8737o5kqtv.fsf@tromey.com> Cc: Tom Tromey , "gdb-patches@sourceware.org" From: Pedro Alves Message-ID: Date: Wed, 03 Aug 2016 11:35:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-08/txt/msg00052.txt.bz2 On 08/03/2016 09:07 AM, Yao Qi wrote: > On Mon, Jul 25, 2016 at 12:04 PM, Pedro Alves wrote: >>> >>> The reason I suggested that way is that the exception may be thrown out in >>> find_frame_funname after the memory is allocated for funname, so we need >>> xfree in CATCH, and also need xfree afterwards. >> >> I disagree. In general, I think that up until the called function does a normal > > What do you disagree on? That it's the caller's responsibility to free an output parameter of a called function that throws. Or more generally, that the state of an output parameter as observed in the caller is determinate when the callee throws. > >> return, the memory for output parameters is owned by the called function. >> A normal return then transfers ownership of the output parameters' memory >> to the caller. > > Yes, so we need xfree after find_frame_funname on normal return. That's what Tromey's patch does. > That is what I suggested. You suggested to free it _also_ when the exception is thrown. That's where my disagreement lies. > > We need to free the memory referenced by output parameter when exception > is thrown too. This. > The point in question is that who is responsible to free the > memory referenced by output parameter. Right. > In Tom's patch, they are freed in > the caller in normal return, so it is reasonable to free them in the caller in > exception return as well, because it is not specified that find_frame_funname > frees the memory on exception. I don't think it needs to be explicitly specified, because I think it should be the behavior or any function that has output parameters. It's unsafe otherwise, because when an exception is thrown from inside a callee, the caller has no idea whether the output parameter has been definitely assigned to. - the callee might throw an exception before the output parameter pointer is ever written to. - the output parameter pointer may have been initialized but now be dangling at the point the exception is thrown inside callee - the callee freed it before throwing. So the exception path (usually the cleanup) in the caller could try to use a dangling pointer (or even a partially constructed object). Basically, this, where foo returns through an output param: extern void foo (char **ret); char *ret; old_chain = make_cleanup (xfree, ret); foo (&ret); do_cleanups (old_chain); ... is as broken as this obviously broken one, which is the exact same except that it returns through normal return: extern char *foo (void); char *ret; old_chain = make_cleanup (xfree, ret); ret = foo (); do_cleanups (old_chain); > >> >> So I think that it's find_frame_funname that should be responsible for making >> sure that memory for output parameters is cleaned up on exception, or be >> written in a way that never throws after the memory allocation, which it may be >> already, but I haven't checked in detail. >> > > If you think it is find_frame_funname's responsibility to free memory on > exception, that is fine. We should document this behaviour for > find_frame_funname and guarantee that find_frame_funname behaves > that way. However, we are not sure current find_frame_funname behaves that > way, because exception may be thrown in cp_remove_params. IMO that becomes an unrelated, preexisting problem. I don't think we should require that all the functions (and their callees, transitively) called by all patches are first inspected for leaks and fixed. Thanks, Pedro Alves