[PATCH] Fix memory leak in C++ pretty printer

[PATCH] Fix memory leak in C++ pretty printer

[Patrick Palka]

In gcc/cp/error.c we initialize the C++ pretty printer object twice:
first during statics initialization and later in a placement-new in
init_error().  This double-initialization causes a memory leak of about
7kb according to valgrind.  I don't see a reason to initialize the
object a second time so I elected to remove init_error().

Does the patch look OK after testing?

gcc/cp/ChangeLog:

	* cp-tree.h (init_error): Remove declaration.
	* error.c (scratch_pretty_printer): Rename to ...
	(actual_pretty_printer): ... this.
	(cxx_pp): Constify and update accordingly.
	(init_error): Remove definition.
	* lex.c (cxx_init): Do not call init_error.
---
 gcc/cp/cp-tree.h |  1 -
 gcc/cp/error.c   | 14 ++------------
 gcc/cp/lex.c     |  1 -
 3 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 4136d98..c51f88f 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5505,7 +5505,6 @@ extern tree vtv_finish_verification_constructor_init_function (tree);
 extern bool cp_omp_mappable_type		(tree);
 
 /* in error.c */
-extern void init_error				(void);
 extern const char *type_as_string		(tree, int);
 extern const char *type_as_string_translate	(tree, int);
 extern const char *decl_as_string		(tree, int);
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index ce43f86..0d98a3c 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -54,8 +54,8 @@ along with GCC; see the file COPYING3.  If not see
    tree -> string functions that are occasionally called from the
    debugger or by the front-end for things like
    __PRETTY_FUNCTION__.  */
-static cxx_pretty_printer scratch_pretty_printer;
-static cxx_pretty_printer * cxx_pp = &scratch_pretty_printer;
+static cxx_pretty_printer actual_pretty_printer;
+static cxx_pretty_printer * const cxx_pp = &actual_pretty_printer;
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
@@ -140,16 +140,6 @@ cxx_initialize_diagnostics (diagnostic_context *context)
   diagnostic_format_decoder (context) = cp_printer;
 }
 
-/* Initialize the global cxx_pp that is used as the memory store for
-   the string representation of C++ AST.  See the description of
-   cxx_pp above.  */
-
-void
-init_error (void)
-{
-  new (cxx_pp) cxx_pretty_printer ();
-}
-
 /* Dump a scope, if deemed necessary.  */
 
 static void
diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c
index 0fced4f..bde15bc 100644
--- a/gcc/cp/lex.c
+++ b/gcc/cp/lex.c
@@ -259,7 +259,6 @@ cxx_init (void)
   init_cp_semantics ();
   init_operators ();
   init_method ();
-  init_error ();
 
   current_function_decl = NULL;
 
-- 
2.4.0.rc2.31.g7c597ef

Re: [PATCH] Fix memory leak in C++ pretty printer

[Manuel López-Ibáñez]

On 11/05/15 03:34, Patrick Palka wrote:
> In gcc/cp/error.c we initialize the C++ pretty printer object twice:
> first during statics initialization and later in a placement-new in
> init_error().  This double-initialization causes a memory leak of about
> 7kb according to valgrind.  I don't see a reason to initialize the
> object a second time so I elected to remove init_error().

I seem to remember there is some issue with the constructors when using static 
initialization that requires the placement-new. We also do the placement-new 
dance in the other FEs and the reason should be the same.

My preference would be to replace the static with a pointer and placement-new 
with proper new and delete, but see: 
https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00910.html

If you change it here, please change it everywhere else where we use 
placement-new, such that all FEs are consistent on this.

Cheers,

Manuel.

Re: [PATCH] Fix memory leak in C++ pretty printer

[Jason Merrill]

On 05/11/2015 08:03 AM, Manuel López-Ibáñez wrote:
> My preference would be to replace the static with a pointer and
> placement-new with proper new and delete, but see:
> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00910.html

Agreed.

Jason

Re: [PATCH] Fix memory leak in C++ pretty printer

[Jason Merrill]

On 05/11/2015 12:57 PM, Jason Merrill wrote:
> On 05/11/2015 08:03 AM, Manuel López-Ibáñez wrote:
>> My preference would be to replace the static with a pointer and
>> placement-new with proper new and delete

Actually, on second thought, there really doesn't seem to be a need for 
that.  The patch should be OK; if it doesn't work I'd like to know why. 
  I think the existing pattern is just a holdover from the C days.

Jason

Re: [PATCH] Fix memory leak in C++ pretty printer

[Jason Merrill]

On 05/11/2015 02:01 PM, Jason Merrill wrote:
> On 05/11/2015 12:57 PM, Jason Merrill wrote:
>> On 05/11/2015 08:03 AM, Manuel López-Ibáñez wrote:
>>> My preference would be to replace the static with a pointer and
>>> placement-new with proper new and delete
>
> Actually, on second thought, there really doesn't seem to be a need for
> that.  The patch should be OK; if it doesn't work I'd like to know why.
> I think the existing pattern is just a holdover from the C days.

So go ahead and apply the patch.  If you would also make the similar fix 
to other front ends, that would be great, too.

Jason

Re: [PATCH] Fix memory leak in C++ pretty printer

[Manuel López-Ibáñez]

On 11/05/15 03:34, Patrick Palka wrote:
> In gcc/cp/error.c we initialize the C++ pretty printer object twice:
> first during statics initialization and later in a placement-new in
> init_error().  This double-initialization causes a memory leak of about
> 7kb according to valgrind.  I don't see a reason to initialize the
> object a second time so I elected to remove init_error().

I seem to remember there is some issue with the constructors when using static 
initialization that requires the placement-new. We also do the placement-new 
dance in the other FEs and the reason should be the same.

My preference would be to replace the static with a pointer and placement-new 
with proper new and delete, but see: 
https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00910.html

If you change it here, please change it everywhere else where we use 
placement-new, such that all FEs are consistent on this.

Cheers,

Manuel.