This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] PR python/18565 - make Frame.function work for inline frames
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: Tom Tromey <tom at tromey dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 3 Aug 2016 12:35:31 +0100
- Subject: Re: [RFA] PR python/18565 - make Frame.function work for inline frames
- Authentication-results: sourceware.org; auth=none
- References: <1466439050-11330-1-git-send-email-tom@tromey.com> <86ziqfq6sz.fsf@gmail.com> <8737o5kqtv.fsf@tromey.com> <CAH=s-PMf2Encmm7JFLBDdrP6e07fEddCiVZGEfU+rkrunbSm-A@mail.gmail.com> <d861e87d-a94d-5ae9-5d51-3f7f8ad0ccc1@redhat.com> <CAH=s-PNVbSsvus_E2wJ+OcqGyZEhC-hkwR2hQxvzP3MRjGGy_w@mail.gmail.com>
On 08/03/2016 09:07 AM, Yao Qi wrote:
> On Mon, Jul 25, 2016 at 12:04 PM, Pedro Alves <palves@redhat.com> 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