From: Jeff Law <law@redhat.com>
To: David Malcolm <dmalcolm@redhat.com>,
gcc-patches@gcc.gnu.org, jit@gcc.gnu.org
Subject: Re: [PATCH 05/08] PR jit/63854: Fix double-initialization within tree-pretty-print.c
Date: Wed, 01 Jan 2014 00:00:00 -0000 [thread overview]
Message-ID: <547CDD95.5070403@redhat.com> (raw)
In-Reply-To: <1416965978-15582-6-git-send-email-dmalcolm@redhat.com>
On 11/25/14 18:39, David Malcolm wrote:
> Running various JIT testcases under valgrid showed this one-time leak:
>
> 8,464 (336 direct, 8,128 indirect) bytes in 1 blocks are definitely lost in loss record 144 of 147
> at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x5DF4EA0: xcalloc (xmalloc.c:162)
> by 0x5DB0D9C: pretty_printer::pretty_printer(char const*, int) (pretty-print.c:777)
> by 0x54DA67D: __static_initialization_and_destruction_0(int, int) (tree-pretty-print.c:66)
> by 0x54DA6AF: _GLOBAL__sub_I_tree_pretty_print.c (tree-pretty-print.c:3542)
> by 0x309540F2D9: call_init.part.0 (dl-init.c:82)
> by 0x309540F3C2: _dl_init (dl-init.c:34)
> by 0x3095401229: ??? (in /usr/lib64/ld-2.18.so)
> by 0x1: ???
> by 0xFFEFFFFBE: ???
> by 0xFFEFFFFDA: ???
>
> tree-pretty-print.c:66 is:
> static pretty_printer buffer;
>
> pretty-print.c:777 is:
> pretty_printer::pretty_printer (const char *p, int l)
> : buffer (new (XCNEW (output_buffer)) output_buffer ()),
>
> This is the ctor of "buffer" (the pretty_printer) running at startup
> by the dynamic linker, allocating its "buffer" output_buffer field
> and the pair of obstacks it contains.
>
> [Arguably "buffer" is a poor name for a pretty_printer, and these
> should be named "pp" or somesuch, though that seems like it should
> be a separate patch, which I'll do as a followup].
>
> However, this ctor gets rerun the first time that
> maybe_init_pretty_print is called, reallocating the output_buffer and
> its obstacks, leaking the old ones:
>
> static void
> maybe_init_pretty_print (FILE *file)
> {
> if (!initialized)
> {
> new (&buffer) pretty_printer ();
> /* etc */
> }
> /* etc */
> }
>
> This is a one-time leak, so it's worth fixing mostly for the sake of
> keeping the valgrind output clean.
>
> Having objects with non-empty ctors/dtors in a statically-allocated
> section feels like too much C++ to me: we can't rely on the order in
> which they run, and in the past I've been bitten by runtimes that
> didn't support them fully (where the compiler worked, but the
> linker/startup code didn't).
>
> Hence this patch fixes the leak by rewriting the global "buffer" to
> be a pointer, allocating the pp on the heap, eliminating the
> before-main static ctor/dtor pair.
>
> gcc/ChangeLog:
> PR jit/63854
> * tree-pretty-print.c: Eliminate include of <new>.
> (buffer): Convert this variable from a pretty_printer to a
> pretty_printer *.
> (initialized): Eliminate this variable in favor of the NULL-ness
> of "buffer".
> (print_generic_decl): Update for "buffer" becoming a pointer.
> (print_generic_stmt): Likewise.
> (print_generic_stmt_indented): Likewise.
> (print_generic_expr): Likewise.
> (maybe_init_pretty_print): Likewise, allocating "buffer" on the
> heap and using its non-NULL-ness to ensure idempotency.
OK.
Jeff
prev parent reply other threads:[~2014-12-01 21:28 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-01 0:00 [PATCH 00/08] More memory leak fixes David Malcolm
2014-01-01 0:00 ` [PATCH 04/08] PR jit/63854: Remove xstrdup from ipa/cgraph fprintf calls David Malcolm
2014-01-01 0:00 ` [PATCH 01/08] PR jit/63854: Fix leak in tree-ssa-math-opts.c David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 08/08] PR/64003 workaround (uninit memory in i386.md) David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 02/08] PR jit/63854: Fix leak within jit-builtins.c David Malcolm
2014-01-01 0:00 ` [PATCH 07/08] PR jit/63854: Fix leaks in toyvm.c David Malcolm
2014-01-01 0:00 ` [PATCH 03/08] PR jit/63854: Fix leak in real.c for i386:init_ext_80387_constants David Malcolm
2014-01-01 0:00 ` Richard Biener
2014-01-01 0:00 ` [PATCH 06/08] Avoid overuse of name "buffer" in tree-pretty-print.c David Malcolm
2014-01-01 0:00 ` Jeff Law
2014-01-01 0:00 ` [PATCH 05/08] PR jit/63854: Fix double-initialization within tree-pretty-print.c David Malcolm
2014-01-01 0:00 ` Jeff Law [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=547CDD95.5070403@redhat.com \
--to=law@redhat.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).