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

  PR python/18565 notes that calling frame filters don't work properly for
inlined functions.  This happens because Frame.function on an inline
frame will yield the wrong result.  This patch changes this code to use
find_frame_funname instead, which handles inline frames properly.

Built and regtested on x86-64 Fedora 23.

2016-06-20  Tom Tromey  <tom@tromey.com>

	PR python/18565:
	* python/py-frame.c (frapy_function): Use find_frame_funname.

2016-06-20  Tom Tromey  <tom@tromey.com>

	PR python/18565:
	* gdb.python/py-frame-inline.exp: Add Frame.function test.
---
 gdb/ChangeLog                                | 5 +++++
 gdb/python/py-frame.c                        | 6 +++++-
 gdb/testsuite/ChangeLog                      | 5 +++++
 gdb/testsuite/gdb.python/py-frame-inline.exp | 4 ++++
 4 files changed, 19 insertions(+), 1 deletion(-)
  

Tom Tromey <tom@tromey.com> writes:

>    TRY
>      {
> +      char *funname;
> +      enum language funlang;
> +
>        FRAPY_REQUIRE_VALID (self, frame);
>  
> -      sym = find_pc_function (get_frame_address_in_block (frame));
> +      find_frame_funname (frame, &funname, &funlang, &sym);
> +      xfree (funname);
>      }
>    CATCH (except, RETURN_MASK_ALL)
>      {

Call xfree in CATCH block?  Otherwise, patch is good to me.
  

>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:

Yao> Tom Tromey <tom@tromey.com> writes:
>> TRY
>> {
>> +      char *funname;
>> +      enum language funlang;
>> +
>> FRAPY_REQUIRE_VALID (self, frame);
>> 
>> -      sym = find_pc_function (get_frame_address_in_block (frame));
>> +      find_frame_funname (frame, &funname, &funlang, &sym);
>> +      xfree (funname);
>> }
>> CATCH (except, RETURN_MASK_ALL)
>> {

Yao> Call xfree in CATCH block?  Otherwise, patch is good to me.

I looked at this.  I think it's probably better as-is.
My reasoning is that "funname" is initialized by the call to
find_frame_funname and isn't otherwise used.  So, putting the free where
it appears now means that there is no gap between initialization and
free.

On the other hand if it is moved into the catch, then there have to be
two frees, one in the catch and one afterward.

So if it's ok with you, I'm inclined to leave it as-is.

Another option would be to change find_frame_funname to allow a NULL
argument here.  Then this free would not be needed at all.

Let me know what you think.

Tom
  

Sorry, I missed this mail,

On Wed, Jun 22, 2016 at 7:42 PM, Tom Tromey <tom@tromey.com> wrote:
>>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:
>
> Yao> Tom Tromey <tom@tromey.com> writes:
>>> TRY
>>> {
>>> +      char *funname;
>>> +      enum language funlang;
>>> +
>>> FRAPY_REQUIRE_VALID (self, frame);
>>>
>>> -      sym = find_pc_function (get_frame_address_in_block (frame));
>>> +      find_frame_funname (frame, &funname, &funlang, &sym);
>>> +      xfree (funname);
>>> }
>>> CATCH (except, RETURN_MASK_ALL)
>>> {
>
> Yao> Call xfree in CATCH block?  Otherwise, patch is good to me.
>
> I looked at this.  I think it's probably better as-is.
> My reasoning is that "funname" is initialized by the call to
> find_frame_funname and isn't otherwise used.  So, putting the free where
> it appears now means that there is no gap between initialization and
> free.
>

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.
  

On 07/25/2016 11:23 AM, Yao Qi wrote:
> Sorry, I missed this mail,
> 
> On Wed, Jun 22, 2016 at 7:42 PM, Tom Tromey <tom@tromey.com> wrote:
>>>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:
>>
>> Yao> Tom Tromey <tom@tromey.com> writes:
>>>> TRY
>>>> {
>>>> +      char *funname;
>>>> +      enum language funlang;
>>>> +
>>>> FRAPY_REQUIRE_VALID (self, frame);
>>>>
>>>> -      sym = find_pc_function (get_frame_address_in_block (frame));
>>>> +      find_frame_funname (frame, &funname, &funlang, &sym);
>>>> +      xfree (funname);
>>>> }
>>>> CATCH (except, RETURN_MASK_ALL)
>>>> {
>>
>> Yao> Call xfree in CATCH block?  Otherwise, patch is good to me.
>>
>> I looked at this.  I think it's probably better as-is.
>> My reasoning is that "funname" is initialized by the call to
>> find_frame_funname and isn't otherwise used.  So, putting the free where
>> it appears now means that there is no gap between initialization and
>> free.
>>
> 
> 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
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.

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.

Thanks,
Pedro Alves
  

Yao> The reason I suggested that way is that the exception may be thrown
Yao> out in find_frame_funname after the memory is allocated for
Yao> funname, so we need xfree in CATCH, and also need xfree afterwards.

Pedro> I disagree.  In general, I think that up until the called function does a normal
Pedro> return, the memory for output parameters is owned by the called function.
Pedro> A normal return then transfers ownership of the output parameters' memory
Pedro> to the caller.

Pedro> So I think that it's find_frame_funname that should be responsible for making
Pedro> sure that memory for output parameters is cleaned up on exception, or be
Pedro> written in a way that never throws after the memory allocation, which it may be
Pedro> already, but I haven't checked in detail.

I agree with this.

I've just looked into the function and its callers.

Most cases in find_frame_funname clearly set *funname in a spot where an
exception cannot occur.  The one iffy case is:

	  *funname = xstrdup (SYMBOL_PRINT_NAME (func));
[...]
	  if (*funlang == language_cplus)
	    {
	      /* It seems appropriate to use SYMBOL_PRINT_NAME() here,
		 to display the demangled name that we already have
		 stored in the symbol table, but we stored a version
		 with DMGL_PARAMS turned on, and here we don't want to
		 display parameters.  So remove the parameters.  */
	      char *func_only = cp_remove_params (*funname);

I'm not 100% sure that cp_remove_params cannot throw.  However, it's
simple to deal with this by adding a cleanup in find_frame_funname.  I'm
happy to do this if desired.

Another approach might be to have a free_current_contents cleanup at the
start of find_frame_funname and discard it at the exit.  This would
maybe make it a bit safer in the face of future changes.

Alternatively, if we need a try/catch in the caller to possibly free the
function name, then several other callers are incorrect (ada-lang.c and
stack.c).

Tom
  

On 07/25/2016 04:01 PM, Tom Tromey wrote:

> 	  *funname = xstrdup (SYMBOL_PRINT_NAME (func));
> [...]
> 	  if (*funlang == language_cplus)
> 	    {
> 	      /* It seems appropriate to use SYMBOL_PRINT_NAME() here,
> 		 to display the demangled name that we already have
> 		 stored in the symbol table, but we stored a version
> 		 with DMGL_PARAMS turned on, and here we don't want to
> 		 display parameters.  So remove the parameters.  */
> 	      char *func_only = cp_remove_params (*funname);
> 
> I'm not 100% sure that cp_remove_params cannot throw.  
> However, it's
> simple to deal with this by adding a cleanup in find_frame_funname.  I'm
> happy to do this if desired.
> 
> Another approach might be to have a free_current_contents cleanup at the
> start of find_frame_funname and discard it at the exit.  This would
> maybe make it a bit safer in the face of future changes.

Yet another approach would be to push the xstrdup call to after the
cp_remove_params call, and remove the xfree call, something like:

	  if (*funlang == language_cplus)
	    {
	      char *func_only = cp_remove_params (SYMBOL_PRINT_NAME (func));

	      if (func_only)
		*funname = func_only;
	      else
		*funname = xstrdup (SYMBOL_PRINT_NAME (func));
	    }
	  else
	    *funname = xstrdup (SYMBOL_PRINT_NAME (func));
	}


In any case, IMO this would be the subject of a separate patch.

> Alternatively, if we need a try/catch in the caller to possibly free the
> function name, then several other callers are incorrect (ada-lang.c and
> stack.c).

Yeah.  I think that if a function has such a requirement, then it needs
to be clearly documented as that being part of its API contract.
Otherwise, it's too easy for the called function to change in a way that
makes the caller try to free a dangling output pointer.

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yet another approach would be to push the xstrdup call to after the
Pedro> cp_remove_params call, and remove the xfree call, something like:

Yeah.

Pedro> In any case, IMO this would be the subject of a separate patch.

Let me recommend accepting the original patch; and I will write a new
patch to fix this.

Tom
  

On 07/26/2016 02:18 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> Pedro> In any case, IMO this would be the subject of a separate patch.
> 
> Let me recommend accepting the original patch; 

FAOD, the original patch LGTM.  I think we should put it in too.

> and I will write a new patch to fix this.

I'd be super fine with just forgetting about it, but if you
want to, please go ahead.

Thanks,
Pedro Alves
  

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?

> 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 is
what I suggested.

We need to free the memory referenced by output parameter when exception
is thrown too.  The point in question is that who is responsible to free the
memory referenced by output parameter.  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.

>
> 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.
  

On Tue, Jul 26, 2016 at 12:14 PM, Pedro Alves <palves@redhat.com> wrote:
> On 07/25/2016 04:01 PM, Tom Tromey wrote:
>
>>         *funname = xstrdup (SYMBOL_PRINT_NAME (func));
>> [...]
>>         if (*funlang == language_cplus)
>>           {
>>             /* It seems appropriate to use SYMBOL_PRINT_NAME() here,
>>                to display the demangled name that we already have
>>                stored in the symbol table, but we stored a version
>>                with DMGL_PARAMS turned on, and here we don't want to
>>                display parameters.  So remove the parameters.  */
>>             char *func_only = cp_remove_params (*funname);
>>
>> I'm not 100% sure that cp_remove_params cannot throw.
>> However, it's
>> simple to deal with this by adding a cleanup in find_frame_funname.  I'm
>> happy to do this if desired.
>>
>> Another approach might be to have a free_current_contents cleanup at the
>> start of find_frame_funname and discard it at the exit.  This would
>> maybe make it a bit safer in the face of future changes.
>
> Yet another approach would be to push the xstrdup call to after the
> cp_remove_params call, and remove the xfree call, something like:
>
>           if (*funlang == language_cplus)
>             {
>               char *func_only = cp_remove_params (SYMBOL_PRINT_NAME (func));
>
>               if (func_only)
>                 *funname = func_only;
>               else
>                 *funname = xstrdup (SYMBOL_PRINT_NAME (func));
>             }
>           else
>             *funname = xstrdup (SYMBOL_PRINT_NAME (func));
>         }
>
>
> In any case, IMO this would be the subject of a separate patch.

This is the prerequisite of Tom's patch, no?  It guarantees that *FUNNAME
is not allocated if exception is thrown, otherwise *FUNAME is allocated.
Tom's patch relies on this behaviour.

I am OK with Tom's original patch, but we need to fix find_frame_funname
first.
  

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
  

On Wed, Aug 3, 2016 at 12:35 PM, Pedro Alves <palves@redhat.com> wrote:
>
> 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.

I don't intend to inspect and fix all leaks too.

I am OK with Tom's patch.