[01/16] read-md.c: Add various cleanups to ~rtx_reader

Various global data items relating to reading .md files are
currently initialized in rtx_reader::read_md_files, and are
not cleaned up.

The selftests for the RTL frontend require supporting multiple
reader instances being alive one after another in-process, so
this lack of cleanup would become a leak.

To fix this, this patch moves these initializations to the
rtx_reader constructor, and adds matching cleanups to the destructor,
along with a cleanup of m_base_dir.

gcc/ChangeLog:
	* read-md.c (rtx_reader::rtx_reader): Move initializations
	of various global data from rtx_reader::read_md_files to here.
	(rtx_reader::~rtx_reader): Clean up m_base_dir, and various
	global data.
	(rtx_reader::read_md_files): Move initializations of various
	global data from here to the ctor.
---
 gcc/read-md.c | 49 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 14 deletions(-)

On 10/05/2016 06:14 PM, David Malcolm wrote:
> The selftests for the RTL frontend require supporting multiple
> reader instances being alive one after another in-process, so
> this lack of cleanup would become a leak.

> +  /* Initialize global data.  */
> +  obstack_init (&string_obstack);
> +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p, 0);
> +  obstack_init (&ptr_loc_obstack);
> +  joined_conditions = htab_create (161, leading_ptr_hash, leading_ptr_eq_p, 0);
> +  obstack_init (&joined_conditions_obstack);
> +  md_constants = htab_create (31, leading_string_hash,
> +			      leading_string_eq_p, (htab_del) 0);
> +  enum_types = htab_create (31, leading_string_hash,
> +			    leading_string_eq_p, (htab_del) 0);
> +
> +  /* Unlock the stdio streams.  */
> +  unlock_std_streams ();

Hmm, but these are global statics. Shouldn't they first be moved to 
become class members?


Bernd

On Wed, 2016-10-05 at 17:51 +0200, Bernd Schmidt wrote:
> On 10/05/2016 06:14 PM, David Malcolm wrote:
> > The selftests for the RTL frontend require supporting multiple
> > reader instances being alive one after another in-process, so
> > this lack of cleanup would become a leak.
> 
> > +  /* Initialize global data.  */
> > +  obstack_init (&string_obstack);
> > +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p,
> > 0);
> > +  obstack_init (&ptr_loc_obstack);
> > +  joined_conditions = htab_create (161, leading_ptr_hash,
> > leading_ptr_eq_p, 0);
> > +  obstack_init (&joined_conditions_obstack);
> > +  md_constants = htab_create (31, leading_string_hash,
> > +			      leading_string_eq_p, (htab_del) 0);
> > +  enum_types = htab_create (31, leading_string_hash,
> > +			    leading_string_eq_p, (htab_del) 0);
> > +
> > +  /* Unlock the stdio streams.  */
> > +  unlock_std_streams ();
> 
> Hmm, but these are global statics. Shouldn't they first be moved to 
> become class members?

[CCing Richard Sandiford]

I tried to move these into class rtx_reader, but doing so rapidly
became quite invasive, with many of functions in the gen* tools
becoming methods.

Arguably that would be a good thing, but there are a couple of issues:

(a) some of these functions take "vec" arguments; moving them from
static functions to being class methods requires that vec.h has been
included when the relevant class decl is parsed.

(b) rtx_reader turns into a bug dumping ground of methods, for the
union of all of the various gen* tools.

One way to alleviate these issues would be to do the split of
rtx_reader into base_rtx_reader vs rtx_reader from patch 9 of the kit:
  https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html
and perhaps to split out part of read-md.h into a new read-rtl.h.

Before I reorganize the patches, does this approach sound reasonable?

Alternatively, a less invasive approach is to have an accessor for
these fields, so that things using them can get at them via the
rtx_reader_ptr singleton e.g.:

void
grow_string_obstack (char ch)
{
   obstack_1grow (rtx_reader_ptr->get_string_obstack (), ch);
}

and similar.

Thoughts?
Dave

Sorry, haven't had time to read the full series yet, but:

David Malcolm <dmalcolm@redhat.com> writes:
> On Wed, 2016-10-05 at 17:51 +0200, Bernd Schmidt wrote:
>> On 10/05/2016 06:14 PM, David Malcolm wrote:
>> > The selftests for the RTL frontend require supporting multiple
>> > reader instances being alive one after another in-process, so
>> > this lack of cleanup would become a leak.
>> 
>> > +  /* Initialize global data.  */
>> > +  obstack_init (&string_obstack);
>> > +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p,
>> > 0);
>> > +  obstack_init (&ptr_loc_obstack);
>> > +  joined_conditions = htab_create (161, leading_ptr_hash,
>> > leading_ptr_eq_p, 0);
>> > +  obstack_init (&joined_conditions_obstack);
>> > +  md_constants = htab_create (31, leading_string_hash,
>> > +			      leading_string_eq_p, (htab_del) 0);
>> > +  enum_types = htab_create (31, leading_string_hash,
>> > +			    leading_string_eq_p, (htab_del) 0);
>> > +
>> > +  /* Unlock the stdio streams.  */
>> > +  unlock_std_streams ();
>> 
>> Hmm, but these are global statics. Shouldn't they first be moved to 
>> become class members?
>
> [CCing Richard Sandiford]
>
> I tried to move these into class rtx_reader, but doing so rapidly
> became quite invasive, with many of functions in the gen* tools
> becoming methods.

Is that just to avoid introducing explicit references to the global
rtx_reader object in the gen* tools?  If so, then TBH adding those
references sound better to me than tying generator-specific functions
to the rtx reader (not least because most of them do more than just
read rtl).

> Arguably that would be a good thing, but there are a couple of issues:
>
> (a) some of these functions take "vec" arguments; moving them from
> static functions to being class methods requires that vec.h has been
> included when the relevant class decl is parsed.

I don't think including vec.h more often should be a blocker though. :-)

> (b) rtx_reader turns into a bug dumping ground of methods, for the
> union of all of the various gen* tools.
>
> One way to alleviate these issues would be to do the split of
> rtx_reader into base_rtx_reader vs rtx_reader from patch 9 of the kit:
>   https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html
> and perhaps to split out part of read-md.h into a new read-rtl.h.
>
> Before I reorganize the patches, does this approach sound reasonable?
>
> Alternatively, a less invasive approach is to have an accessor for
> these fields, so that things using them can get at them via the
> rtx_reader_ptr singleton e.g.:
>
> void
> grow_string_obstack (char ch)
> {
>    obstack_1grow (rtx_reader_ptr->get_string_obstack (), ch);
> }
>
> and similar.

I think it's OK for the generators to refer rtx_reader_ptr directly.
Obviously that makes the patches more invasive, but hopefully the
extra changes are mechanical.

Thanks,
Richard