From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 28B3A3854809 for ; Mon, 23 Nov 2020 12:56:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 28B3A3854809 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=hubicka@kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 077AC28277E; Mon, 23 Nov 2020 13:56:48 +0100 (CET) Date: Mon, 23 Nov 2020 13:56:47 +0100 From: Jan Hubicka To: Richard Biener Cc: gcc-patches@gcc.gnu.org Subject: Re: Do not leak memory when streaming in ssa names Message-ID: <20201123125647.GC66486@kam.mff.cuni.cz> References: <20201123121852.GB37604@kam.mff.cuni.cz> <20201123124335.GC37604@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-15.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2020 12:56:51 -0000 > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > > Hi, > > > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > > > that allocates space in ssa_names array for 50 names. Later it streams > > > > in the count and calls init_tree_ssa again, this time with correct > > > > count, which is rounded up to 50 and allocated again leaking the first > > > > array. > > > > > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > > > whether ssa names should be initialized. There are few places where we > > > > initialize ssa and only lto streaming is special, so i think extra arg > > > > works well here. > > > > > > > > I am not quite sure about the 50MB default. I think it was made before > > > > ggc_free was implemented (so resizing vectors was expensive) and when we > > > > had only one function in SSA form at a time. Most of functions in C++ > > > > will probably need fewer than 50 SSA names until inlining. > > > > > > > > This saves 21MB of WPA linking libxul. > > > > > > Why do we input SSA names at WPA? > > > > To ICF compare function bodies. It tends to load tons of small bodies. > > We also may load body when merging BB profile etc. > > > > > > > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? > > > > > > I think calling init_ssanames again is bogus. Please simply > > > do that in input_ssa_names after we know how many we need > > > adding a number of SSA names to reserve argument to init_tree_ssa > > > and passing that down instead. > > > > I tried that originally but it hit ICE in init_ssa_operands. Seems that > > reloacating them is last change I need though. Does this look better? > > Patch below looks completely unrelated but is OK as well if it passes > testing ;) Glad to hear since it is in mainline for a while :) Here is correct pathc. Sorry. * lto-streamer-in.c (input_cfg): Do not init ssa operands. (input_function): Do not init tree_ssa and set in_ssa_p. (input_ssa_names): Do it here. * tree-ssa.c (init_tree_ssa): Add additional SIZE parameter, default to 0 * tree-ssanames.c (init_ssanames): Do not round size up to 50, allocate precisely. diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c index 64037f74ad3..a20d64b0089 100644 --- a/gcc/lto-streamer-in.c +++ b/gcc/lto-streamer-in.c @@ -1017,7 +1017,6 @@ input_cfg (class lto_input_block *ib, class data_in *data_in, int index; init_empty_tree_cfg_for_function (fn); - init_ssa_operands (fn); profile_status_for_fn (fn) = streamer_read_enum (ib, profile_status_d, PROFILE_LAST); @@ -1144,7 +1143,9 @@ input_ssa_names (class lto_input_block *ib, class data_in *data_in, unsigned int i, size; size = streamer_read_uhwi (ib); - init_ssanames (fn, size); + init_tree_ssa (fn, size); + cfun->gimple_df->in_ssa_p = true; + init_ssa_operands (fn); i = streamer_read_uhwi (ib); while (i) @@ -1384,9 +1385,6 @@ input_function (tree fn_decl, class data_in *data_in, push_struct_function (fn_decl); fn = DECL_STRUCT_FUNCTION (fn_decl); - init_tree_ssa (fn); - /* We input IL in SSA form. */ - cfun->gimple_df->in_ssa_p = true; gimple_register_cfg_hooks (); diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index b44361f8244..24a5154620d 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -1218,15 +1218,16 @@ err: # pragma GCC diagnostic pop #endif -/* Initialize global DFA and SSA structures. */ +/* Initialize global DFA and SSA structures. + If SIZE is non-zero allocated ssa names array of a given size. */ void -init_tree_ssa (struct function *fn) +init_tree_ssa (struct function *fn, int size) { fn->gimple_df = ggc_cleared_alloc (); fn->gimple_df->default_defs = hash_table::create_ggc (20); pt_solution_reset (&fn->gimple_df->escaped); - init_ssanames (fn, 0); + init_ssanames (fn, size); } /* Deallocate memory associated with SSA data structures for FNDECL. */ diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index 6ac97fe39c4..ec4681f85cb 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -77,10 +77,10 @@ unsigned int ssa_name_nodes_created; void init_ssanames (struct function *fn, int size) { - if (size < 50) - size = 50; - - vec_alloc (SSANAMES (fn), size); + if (!size) + vec_alloc (SSANAMES (fn), 50); + else + vec_safe_reserve (SSANAMES (fn), size, true); /* Version 0 is special, so reserve the first slot in the table. Though currently unused, we may use version 0 in alias analysis as part of