| Message ID | 1431308052-31361-1-git-send-email-patrick@parcs.ath.cx |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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
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
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;