Re: [RFA] PR python/18565 - make Frame.function work for inline frames

[Pedro Alves <palves@redhat.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