[PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

When r12-4038 introduced the global auto_vec save_opt_decoded_options, 
it broke compilers configured with --enable-gather-detailed-mem-stats, 
due to the memory descriptors getting discarded before the auto_vec was 
destroyed.  Attached below are two approaches to making this work, 
either by using the init_priority attribute, or turning vec_mem_desc 
into a singleton function.  I prefer the first one, primarily because it 
doesn't require auto_vec variables to force immediate allocation.  It 
relies on a G++ extension, but I figure that's OK for code that is only 
exercised with a debugging configure flag.

Thoughts?  Either one OK for trunk?

[-- Attachment #2: 0001-vec-Fix-enable-gather-detailed-mem-stats.patch --]
[-- Type: text/x-patch, Size: 1170 bytes --]

From b12018cb7842a0bab1f5bcf259f73ec17186f4ad Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Sat, 2 Oct 2021 05:45:02 -0400
Subject: [PATCH 1/2] vec: Fix --enable-gather-detailed-mem-stats
To: gcc-patches@gcc.gnu.org

When r12-4038 introduced the global auto_vec save_opt_decoded_options, it
broke compilers configured with --enable-gather-detailed-mem-stats due to
the memory descriptors getting discarded before the auto_vec was destroyed.
We can fix the ordering of construction/destruction using the init_priority
attribute.

gcc/ChangeLog:

	* vec.c (vec_mem_desc): Add init_priority attribute.
---
 gcc/vec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gcc/vec.c b/gcc/vec.c
index 6d767cc12c1..715460397c6 100644
--- a/gcc/vec.c
+++ b/gcc/vec.c
@@ -111,6 +111,11 @@ public:
 };
 
 /* Vector memory description.  */
+#if GATHER_STATISTICS
+/* With --enable-gather-detailed-mem-stats, this needs to be constructed before
+   any auto_vec variables.  The number 237 is entirely arbitrary.  */
+[[gnu::init_priority (237)]]
+#endif
 static mem_alloc_description <vec_usage> vec_mem_desc;
 
 /* Account the overhead.  */
-- 
2.27.0


[-- Attachment #3: 0002-vec-make-vec_mem_desc-a-singleton-function.patch --]
[-- Type: text/x-patch, Size: 3944 bytes --]

From 7641bceec2cb53e76c57bd70c38f390610bb82b6 Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Fri, 1 Oct 2021 16:09:57 -0400
Subject: [PATCH 2/2] vec: make vec_mem_desc a singleton function
To: gcc-patches@gcc.gnu.org

When r12-4038 introduced the global auto_vec save_opt_decoded_options, it
broke compilers configured with --enable-gather-detailed-mem-stats due to
the memory descriptors getting discarded before the auto_vec was destroyed.
We can fix the ordering of construction/destruction by turning vec_mem_desc
into a singleton function.

For this to work, the construction of save_opt_decoded_options needs to
allocate immediately, so that allocation calls vec_mem_desc.

gcc/ChangeLog:

	* vec.c (vec_mem_desc): Change to function.
	(vec_prefix::register_overhead): Adjust.
	(vec_prefix::release_overhead): Adjust.
	(dump_vec_loc_statistics): Adjust.
	* toplev.c (save_opt_decoded_options): Allocate.
---
 gcc/toplev.c |  7 ++++++-
 gcc/vec.c    | 28 +++++++++++++++-------------
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/gcc/toplev.c b/gcc/toplev.c
index ec9f998a49b..727a9972dd0 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -122,7 +122,12 @@ struct cl_decoded_option *save_decoded_options;
 unsigned int save_decoded_options_count;
 
 /* Vector of saved Optimization decoded command line options.  */
-auto_vec<cl_decoded_option> save_opt_decoded_options;
+auto_vec<cl_decoded_option> save_opt_decoded_options
+#if GATHER_STATISTICS
+/* Force allocation so vec_mem_desc is called.  */
+(1)
+#endif
+;
 
 /* Used to enable -fvar-tracking, -fweb and -frename-registers according
    to optimize in process_options ().  */
diff --git a/gcc/vec.c b/gcc/vec.c
index 715460397c6..4858753223c 100644
--- a/gcc/vec.c
+++ b/gcc/vec.c
@@ -110,13 +110,15 @@ public:
   size_t m_element_size;
 };
 
-/* Vector memory description.  */
-#if GATHER_STATISTICS
-/* With --enable-gather-detailed-mem-stats, this needs to be constructed before
-   any auto_vec variables.  The number 237 is entirely arbitrary.  */
-[[gnu::init_priority (237)]]
-#endif
-static mem_alloc_description <vec_usage> vec_mem_desc;
+/* Vector memory description.  This is a singleton function rather than a
+   variable so that the object's [cd]tor are properly ordered relative to any
+   auto_vec variables.  */
+static mem_alloc_description <vec_usage> &
+vec_mem_desc ()
+{
+  static mem_alloc_description<vec_usage> m;
+  return m;
+}
 
 /* Account the overhead.  */
 
@@ -124,10 +126,10 @@ void
 vec_prefix::register_overhead (void *ptr, size_t elements,
 			       size_t element_size MEM_STAT_DECL)
 {
-  vec_mem_desc.register_descriptor (ptr, VEC_ORIGIN, false
+  vec_mem_desc ().register_descriptor (ptr, VEC_ORIGIN, false
 				    FINAL_PASS_MEM_STAT);
   vec_usage *usage
-    = vec_mem_desc.register_instance_overhead (elements * element_size, ptr);
+    = vec_mem_desc ().register_instance_overhead (elements * element_size, ptr);
   usage->m_element_size = element_size;
   usage->m_items += elements;
   if (usage->m_items_peak < usage->m_items)
@@ -140,10 +142,10 @@ void
 vec_prefix::release_overhead (void *ptr, size_t size, size_t elements,
 			      bool in_dtor MEM_STAT_DECL)
 {
-  if (!vec_mem_desc.contains_descriptor_for_instance (ptr))
-    vec_mem_desc.register_descriptor (ptr, VEC_ORIGIN,
+  if (!vec_mem_desc ().contains_descriptor_for_instance (ptr))
+    vec_mem_desc ().register_descriptor (ptr, VEC_ORIGIN,
 				      false FINAL_PASS_MEM_STAT);
-  vec_usage *usage = vec_mem_desc.release_instance_overhead (ptr, size,
+  vec_usage *usage = vec_mem_desc ().release_instance_overhead (ptr, size,
 							     in_dtor);
   usage->m_items -= elements;
 }
@@ -178,7 +180,7 @@ vec_prefix::calculate_allocation_1 (unsigned alloc, unsigned desired)
 void
 dump_vec_loc_statistics (void)
 {
-  vec_mem_desc.dump (VEC_ORIGIN);
+  vec_mem_desc ().dump (VEC_ORIGIN);
 }
 
 #if CHECKING_P
-- 
2.27.0

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Iain Sandoe]

Hi Jason,

> On 4 Oct 2021, at 19:27, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> When r12-4038 introduced the global auto_vec save_opt_decoded_options, it broke compilers configured with --enable-gather-detailed-mem-stats, due to the memory descriptors getting discarded before the auto_vec was destroyed.  Attached below are two approaches to making this work, either by using the init_priority attribute, or turning vec_mem_desc into a singleton function.  I prefer the first one, primarily because it doesn't require auto_vec variables to force immediate allocation.  It relies on a G++ extension, but I figure that's OK for code that is only exercised with a debugging configure flag.

I suspect the problem is not necessarily that it’s a G++ extension, (init priority has some support elsewhere) - but that some targets (with non-binutils ld) cannot support it [between multiple TUs at least] even with a native G++ (Darwin at least cannot).  OTOH, there are worse broken things from this than a gathering of stats…
Iain

> Thoughts?  Either one OK for trunk?<0001-vec-Fix-enable-gather-detailed-mem-stats.patch><0002-vec-make-vec_mem_desc-a-singleton-function.patch>

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Jason Merrill]

On 10/4/21 14:37, Iain Sandoe wrote:
> Hi Jason,
> 
>> On 4 Oct 2021, at 19:27, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>
>> When r12-4038 introduced the global auto_vec save_opt_decoded_options, it broke compilers configured with --enable-gather-detailed-mem-stats, due to the memory descriptors getting discarded before the auto_vec was destroyed.  Attached below are two approaches to making this work, either by using the init_priority attribute, or turning vec_mem_desc into a singleton function.  I prefer the first one, primarily because it doesn't require auto_vec variables to force immediate allocation.  It relies on a G++ extension, but I figure that's OK for code that is only exercised with a debugging configure flag.
> 
> I suspect the problem is not necessarily that it’s a G++ extension, (init priority has some support elsewhere) - but that some targets (with non-binutils ld) cannot support it [between multiple TUs at least] even with a native G++ (Darwin at least cannot).  OTOH, there are worse broken things from this than a gathering of stats…

Hmm, that was previously handled for other linkers with the collect2 
wrapper.  I haven't followed what has happened with collect2 in recent 
years, does Darwin not use it?

Jason

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Iain Sandoe]

> On 4 Oct 2021, at 22:27, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> On 10/4/21 14:37, Iain Sandoe wrote:
>> Hi Jason,
>>> On 4 Oct 2021, at 19:27, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> When r12-4038 introduced the global auto_vec save_opt_decoded_options, it broke compilers configured with --enable-gather-detailed-mem-stats, due to the memory descriptors getting discarded before the auto_vec was destroyed.  Attached below are two approaches to making this work, either by using the init_priority attribute, or turning vec_mem_desc into a singleton function.  I prefer the first one, primarily because it doesn't require auto_vec variables to force immediate allocation.  It relies on a G++ extension, but I figure that's OK for code that is only exercised with a debugging configure flag.
>> I suspect the problem is not necessarily that it’s a G++ extension, (init priority has some support elsewhere) - but that some targets (with non-binutils ld) cannot support it [between multiple TUs at least] even with a native G++ (Darwin at least cannot).  OTOH, there are worse broken things from this than a gathering of stats…
> 
> Hmm, that was previously handled for other linkers with the collect2 wrapper.  I haven't followed what has happened with collect2 in recent years, does Darwin not use it?

It does use collect2, but init_priority is, nevertheless disabled for the target; I will investigate some more.
thanks for the head’s up,
Iain

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Richard Biener]

On Mon, Oct 4, 2021 at 8:28 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> When r12-4038 introduced the global auto_vec save_opt_decoded_options,
> it broke compilers configured with --enable-gather-detailed-mem-stats,
> due to the memory descriptors getting discarded before the auto_vec was
> destroyed.  Attached below are two approaches to making this work,
> either by using the init_priority attribute, or turning vec_mem_desc
> into a singleton function.  I prefer the first one, primarily because it
> doesn't require auto_vec variables to force immediate allocation.  It
> relies on a G++ extension, but I figure that's OK for code that is only
> exercised with a debugging configure flag.
>
> Thoughts?  Either one OK for trunk?

Hmm, isn't the way to fix this to turn the global auto_vec into
vec<> *x and allocate it at runtime (thus explicitly mange its
lifetime?).  We don't want global CTORs/DTORs in general
because of startup cost and of course those pesky ordering issues...

Richard.

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Richard Biener]

On Tue, Oct 5, 2021 at 1:27 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Oct 4, 2021 at 8:28 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > When r12-4038 introduced the global auto_vec save_opt_decoded_options,
> > it broke compilers configured with --enable-gather-detailed-mem-stats,
> > due to the memory descriptors getting discarded before the auto_vec was
> > destroyed.  Attached below are two approaches to making this work,
> > either by using the init_priority attribute, or turning vec_mem_desc
> > into a singleton function.  I prefer the first one, primarily because it
> > doesn't require auto_vec variables to force immediate allocation.  It
> > relies on a G++ extension, but I figure that's OK for code that is only
> > exercised with a debugging configure flag.
> >
> > Thoughts?  Either one OK for trunk?
>
> Hmm, isn't the way to fix this to turn the global auto_vec into
> vec<> *x and allocate it at runtime (thus explicitly mange its
> lifetime?).  We don't want global CTORs/DTORs in general
> because of startup cost and of course those pesky ordering issues...

Oh, and maybe we can make

 static mem_alloc_description <vec_usage> vec_mem_desc;

statically initialized with some C++?  (constexpr? constinit? whatever?)

> Richard.

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Jason Merrill]

On 10/5/21 07:27, Richard Biener wrote:
> On Mon, Oct 4, 2021 at 8:28 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> When r12-4038 introduced the global auto_vec save_opt_decoded_options,
>> it broke compilers configured with --enable-gather-detailed-mem-stats,
>> due to the memory descriptors getting discarded before the auto_vec was
>> destroyed.  Attached below are two approaches to making this work,
>> either by using the init_priority attribute, or turning vec_mem_desc
>> into a singleton function.  I prefer the first one, primarily because it
>> doesn't require auto_vec variables to force immediate allocation.  It
>> relies on a G++ extension, but I figure that's OK for code that is only
>> exercised with a debugging configure flag.
>>
>> Thoughts?  Either one OK for trunk?
> 
> Hmm, isn't the way to fix this to turn the global auto_vec into
> vec<> *x and allocate it at runtime (thus explicitly mange its
> lifetime?).  We don't want global CTORs/DTORs in general
> because of startup cost and of course those pesky ordering issues...

That is the usual approach, yes.  I was giving up on that, but perhaps 
it's better to stick with it.  Martin, want to make that fix for 
save_opt_decoded_options?

> Oh, and maybe we can make
> 
>  static mem_alloc_description <vec_usage> vec_mem_desc;
> 
> statically initialized with some C++?  (constexpr? constinit? whatever?
It can't be statically initialized, because it needs to allocate 
multiple maps.

Jason

Re: [PATCH RFA] vec: Fix --enable-gather-detailed-mem-stats

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On 10/5/21 20:44, Jason Merrill wrote:
> That is the usual approach, yes.  I was giving up on that, but perhaps it's better to stick with it.  Martin, want to make that fix for save_opt_decoded_options?

Yes, I'm going to install the following patch once it survives regression tests
and bootstrap.

Martin

[-- Attachment #2: 0001-build-Fix-enable-gather-detailed-mem-stats.patch --]
[-- Type: text/x-patch, Size: 3335 bytes --]

From 16f245cc4a738480c5dbdcc700c1859365a0eab5 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 7 Oct 2021 12:29:15 +0200
Subject: [PATCH] build: Fix --enable-gather-detailed-mem-stats

gcc/c-family/ChangeLog:

	* c-common.c (parse_optimize_options): Make
	save_opt_decoded_options a pointer type.

gcc/ChangeLog:

	* toplev.c (toplev::main): Make
	save_opt_decoded_options a pointer type
	* toplev.h: Likewise.
---
 gcc/c-family/c-common.c | 4 ++--
 gcc/toplev.c            | 5 +++--
 gcc/toplev.h            | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9d19e352725..32c7e3e8972 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5921,7 +5921,7 @@ parse_optimize_options (tree args, bool attr_p)
   decoded_options_count = j;
 
   /* Merge the decoded options with save_decoded_options.  */
-  unsigned save_opt_count = save_opt_decoded_options.length ();
+  unsigned save_opt_count = save_opt_decoded_options->length ();
   unsigned merged_decoded_options_count
     = save_opt_count + decoded_options_count;
   cl_decoded_option *merged_decoded_options
@@ -5929,7 +5929,7 @@ parse_optimize_options (tree args, bool attr_p)
 
   /* Note the first decoded_options is used for the program name.  */
   for (unsigned i = 0; i < save_opt_count; ++i)
-    merged_decoded_options[i + 1] = save_opt_decoded_options[i];
+    merged_decoded_options[i + 1] = (*save_opt_decoded_options)[i];
   for (unsigned i = 1; i < decoded_options_count; ++i)
     merged_decoded_options[save_opt_count + i] = decoded_options[i];
 
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 70769087c13..ecb2b694970 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -117,7 +117,7 @@ struct cl_decoded_option *save_decoded_options;
 unsigned int save_decoded_options_count;
 
 /* Vector of saved Optimization decoded command line options.  */
-auto_vec<cl_decoded_option> save_opt_decoded_options;
+vec<cl_decoded_option> *save_opt_decoded_options;
 
 /* Used to enable -fvar-tracking, -fweb and -frename-registers according
    to optimize in process_options ().  */
@@ -2320,10 +2320,11 @@ toplev::main (int argc, char **argv)
 						&save_decoded_options_count);
 
   /* Save Optimization decoded options.  */
+  save_opt_decoded_options = new vec<cl_decoded_option> ();
   for (unsigned i = 1; i < save_decoded_options_count; ++i)
     if (save_decoded_options[i].opt_index < cl_options_count
 	&& cl_options[save_decoded_options[i].opt_index].flags & CL_OPTIMIZATION)
-      save_opt_decoded_options.safe_push (save_decoded_options[i]);
+      save_opt_decoded_options->safe_push (save_decoded_options[i]);
 
   /* Perform language-specific options initialization.  */
   lang_hooks.init_options (save_decoded_options_count, save_decoded_options);
diff --git a/gcc/toplev.h b/gcc/toplev.h
index c44c5ff926a..493f7eb5ad6 100644
--- a/gcc/toplev.h
+++ b/gcc/toplev.h
@@ -23,7 +23,7 @@ along with GCC; see the file COPYING3.  If not see
 /* Decoded options, and number of such options.  */
 extern struct cl_decoded_option *save_decoded_options;
 extern unsigned int save_decoded_options_count;
-extern auto_vec<cl_decoded_option> save_opt_decoded_options;
+extern vec<cl_decoded_option> *save_opt_decoded_options;
 
 class timer;
 
-- 
2.33.0