Destroy allocated values when exiting GDB (was: Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1)

  On Thursday, March 28 2019, Pedro Alves wrote:

> On 03/27/2019 12:57 PM, Tom Tromey wrote:
>> Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of
>> Sergio> collective investigation.  I summarized what we found here:
>> Sergio>   https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25
>> 
>> Thanks, that's very interesting.
>> I suppose either better control over the order of destruction is needed,
>> or maybe finalize_python should clear gdb_python_initialized and then
>> this should be checked in xmethod destructor.
> I think the former is better.  I think we should put an 
>   all_values.clear ();
> call in quit_force, before the do_final_cleanups call.  Even
> better, add a new finalize_values function next to
> _initialize_values, and call that.

How does this look?

From b88067edfb2289cd5af3c9b8984b0b0354c965db Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 29 Mar 2019 17:34:54 -0400
Subject: [PATCH] Destroy allocated values when exiting GDB

When the user exits GDB, we might still have some allocated values in
the chain, which, in specific scenarios, can cause problems when GDB
attempts to destroy them in "quit_force".  For example, see the bug
reported at:

  https://bugzilla.redhat.com/show_bug.cgi?id=1690120

And the thread starting at:

  https://sourceware.org/ml/gdb-patches/2019-03/msg00475.html
  Message-ID: <87r2azkhmq.fsf@redhat.com>

In order to avoid that, and to be more aware of our allocated
resources, this commit implements a new function "finalize_values" and
calls it from inside "quit_force".

Tested by the BuildBot.

2019-03-29  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* top.c (quit_force): Call 'finalize_values'.
	* value.c (finalize_values): New function.
	* value.h (finalize_values): Declare.
---
 gdb/top.c   | 3 +++
 gdb/value.c | 8 ++++++++
 gdb/value.h | 4 ++++
 3 files changed, 15 insertions(+)
  

On 03/29/2019 09:43 PM, Sergio Durigan Junior wrote:

> 
> diff --git a/gdb/top.c b/gdb/top.c
> index b10b0649e9..ffbe8e517f 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1672,6 +1672,9 @@ quit_force (int *exit_arg, int from_tty)
>      }
>    END_CATCH
>  
> +  /* Destroy any values currently allocated.  */
> +  finalize_values ();

I think that someone reading this without context may not realize
why we need to do that.  How about extending the comment, like:

  /* Destroy any values currently allocated now instead of leaving it
     to global destructors, because that may be too late.  For
     example, the destructors of xmethod values call into the Python
     runtime, which is finalized via a final cleanup.  */

> diff --git a/gdb/value.h b/gdb/value.h
> index d3905cc354..7853950ca3 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -1189,4 +1189,8 @@ extern struct value *call_xmethod (struct value *method,
>  extern int value_union_variant (struct type *union_type,
>  				const gdb_byte *contents);
>  
> +/* Destroy the values currently allocated.  This is mostly called when
> +   GDB is exiting (e.g., on quit_force).  */

s/mostly//

> +extern void finalize_values ();
> +
>  #endif /* !defined (VALUE_H) */

LGTM with those changes.

Thanks,
Pedro Alves
  

On Monday, April 01 2019, Pedro Alves wrote:

> On 03/29/2019 09:43 PM, Sergio Durigan Junior wrote:
>
>> 
>> diff --git a/gdb/top.c b/gdb/top.c
>> index b10b0649e9..ffbe8e517f 100644
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -1672,6 +1672,9 @@ quit_force (int *exit_arg, int from_tty)
>>      }
>>    END_CATCH
>>  
>> +  /* Destroy any values currently allocated.  */
>> +  finalize_values ();
>
> I think that someone reading this without context may not realize
> why we need to do that.  How about extending the comment, like:
>
>   /* Destroy any values currently allocated now instead of leaving it
>      to global destructors, because that may be too late.  For
>      example, the destructors of xmethod values call into the Python
>      runtime, which is finalized via a final cleanup.  */

Done.

>> diff --git a/gdb/value.h b/gdb/value.h
>> index d3905cc354..7853950ca3 100644
>> --- a/gdb/value.h
>> +++ b/gdb/value.h
>> @@ -1189,4 +1189,8 @@ extern struct value *call_xmethod (struct value *method,
>>  extern int value_union_variant (struct type *union_type,
>>  				const gdb_byte *contents);
>>  
>> +/* Destroy the values currently allocated.  This is mostly called when
>> +   GDB is exiting (e.g., on quit_force).  */
>
> s/mostly//

Done.

>> +extern void finalize_values ();
>> +
>>  #endif /* !defined (VALUE_H) */
>
> LGTM with those changes.

Thanks for the review.  Pushed:

9d1447e09d4aa673826039321163b5a684e8e043