* [PATCH] Fix memory leak in C++ pretty printer
@ 2015-05-11 1:34 Patrick Palka
2015-05-11 13:04 ` Manuel López-Ibáñez
2015-05-11 13:30 ` Manuel López-Ibáñez
0 siblings, 2 replies; 6+ messages in thread
From: Patrick Palka @ 2015-05-11 1:34 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in C++ pretty printer
2015-05-11 1:34 [PATCH] Fix memory leak in C++ pretty printer Patrick Palka
@ 2015-05-11 13:04 ` Manuel López-Ibáñez
2015-05-11 17:57 ` Jason Merrill
2015-05-11 13:30 ` Manuel López-Ibáñez
1 sibling, 1 reply; 6+ messages in thread
From: Manuel López-Ibáñez @ 2015-05-11 13:04 UTC (permalink / raw)
To: Patrick Palka, gcc-patches; +Cc: jason
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in C++ pretty printer
2015-05-11 1:34 [PATCH] Fix memory leak in C++ pretty printer Patrick Palka
2015-05-11 13:04 ` Manuel López-Ibáñez
@ 2015-05-11 13:30 ` Manuel López-Ibáñez
1 sibling, 0 replies; 6+ messages in thread
From: Manuel López-Ibáñez @ 2015-05-11 13:30 UTC (permalink / raw)
To: Patrick Palka, gcc-patches; +Cc: jason
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in C++ pretty printer
2015-05-11 13:04 ` Manuel López-Ibáñez
@ 2015-05-11 17:57 ` Jason Merrill
2015-05-11 18:01 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2015-05-11 17:57 UTC (permalink / raw)
To: Manuel López-Ibáñez, Patrick Palka, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in C++ pretty printer
2015-05-11 17:57 ` Jason Merrill
@ 2015-05-11 18:01 ` Jason Merrill
2015-05-20 14:58 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2015-05-11 18:01 UTC (permalink / raw)
To: Manuel López-Ibáñez, Patrick Palka, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in C++ pretty printer
2015-05-11 18:01 ` Jason Merrill
@ 2015-05-20 14:58 ` Jason Merrill
0 siblings, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2015-05-20 14:58 UTC (permalink / raw)
To: Manuel López-Ibáñez, Patrick Palka, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-20 14:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-11 1:34 [PATCH] Fix memory leak in C++ pretty printer Patrick Palka
2015-05-11 13:04 ` Manuel López-Ibáñez
2015-05-11 17:57 ` Jason Merrill
2015-05-11 18:01 ` Jason Merrill
2015-05-20 14:58 ` Jason Merrill
2015-05-11 13:30 ` Manuel López-Ibáñez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).