From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67996 invoked by alias); 25 May 2015 10:25:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 67974 invoked by uid 89); 25 May 2015 10:25:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mout.gmx.net Received: from mout.gmx.net (HELO mout.gmx.net) (212.227.17.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 25 May 2015 10:24:56 +0000 Received: from vepi2 ([88.75.104.20]) by mail.gmx.com (mrgmx102) with ESMTPSA (Nemesis) id 0LqW8j-1ZaY9x3Twf-00e7oB; Mon, 25 May 2015 12:24:49 +0200 Date: Mon, 25 May 2015 11:52:00 -0000 From: Andre Vehreschild To: Paul Richard Thomas Cc: "fortran@gcc.gnu.org" , gcc-patches , Damian Rouson Subject: Re: [Patch, fortran] PR66079 - [6 Regression] memory leak with source allocation in internal subprogram Message-ID: <20150525122447.5aafa2da@vepi2> In-Reply-To: References: <20150523195252.22647b46@vepi2> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="MP_/XnNfN5KmngRzakwjE.PUHA7" X-UI-Out-Filterresults: notjunk:1; X-SW-Source: 2015-05/txt/msg02237.txt.bz2 --MP_/XnNfN5KmngRzakwjE.PUHA7 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 7054 Hi Paul, I am not quite happy with the naming of the temporary variable. When I initially set the prefix to "atmp" this was because the variable would be an array most of the time and because of the number appended to it should be unique anyway. However I would like to point out that disclosing an internal implementation detail of the compiler to a future developer looking at the pseudo-code dump will not help (I mean "expr3", here). I would rather use "source" as the prefix now that I think of it with some distance to the original naming. What do you think? Now that the deallocate for source's components is in the patch, I understand why initially the source= preevaluation for derived types with allocatable components was disallowed. Thanks for clarifying that. I wonder though, if we can't do better... Please have a look at the attached patch. It not only renames the temporary variable from "expr3" to "source" (couldn't help, but do it. Please don't be angry :-)), but also adds move semantics to source= expressions for the last object to allocate. I.e., when a scalar source= expression with allocatable components is detected, then its content is "moved" (memcpy'ed) to the last object to allocate instead of being assigned. All former objects to allocate are of course handled like before, i.e., components are allocated and the contents of the source= expression is copied using the assign. But when a move could be done the alloc/dealloc of the components is skipped. With this I hope to safe a lot of mallocs and frees, which are not that cheap. In the most common case where only one object is allocated, there now is only one alloc for the components to get expr3 up and one for the object to allocate. We safe the allocate of the allocatable components in the object to allocate and the free of the source= components. I hope I could make clear what I desire? If not maybe a look into the patch might help. What do you think? The patch of course is only a quick implementation of the idea. Please comment, everyone! Regards, Andre On Mon, 25 May 2015 09:30:34 +0200 Paul Richard Thomas wrote: > Dear All, > > Lets see if I can get it right this time :-) > > Note that I have changed the name of the temporary variable in > trans_allocate from 'atmp' to 'expr3' so that it is not confused with > array temporaries. I am not suree how much of the testcase is > pertinent after the reform of the evaluation of expr3 performed by > Andre. However, there were still memory leaks that are fixed by the > attached patch. > > Bootstrapped and regtested on a current trunk - OK for trunk? > > Paul > > 2015-05-23 Paul Thomas > > PR fortran/66079 > * trans-expr.c (gfc_conv_procedure_call): Allocatable scalar > function results must be freed and nullified after use. Create > a temporary to hold the result to prevent duplicate calls. > * trans-stmt.c (gfc_trans_allocate): Rename temporary variable > as 'expr3'. Deallocate allocatable components of non-variable > expr3s. > > 2015-05-23 Paul Thomas > > PR fortran/66079 > * gfortran.dg/allocatable_scalar_13.f90: New test > > > On 24 May 2015 at 09:51, Paul Richard Thomas > wrote: > > Dear Andre, > > > > I'll put both points right. Thanks for pointing them out. > > > > Cheers > > > > Paul > > > > On 23 May 2015 at 19:52, Andre Vehreschild wrote: > >> Hi Paul, > >> > >> does this patch apply to current trunk cleanly? I get an issue with the > >> last hunk, because all of the prerequisites are gone since r223445. The > >> string copy is completely handled by the trans_assignment at the bottom of > >> the if (code->expr3) block. Therefore I doubt the patches last hunk is > >> needed any longer. > >> > >> Do you have an example why this hunk is needed? > >> > >> Index: gcc/fortran/trans-stmt.c > >> =================================================================== > >> *** gcc/fortran/trans-stmt.c (revision 223233) > >> --- gcc/fortran/trans-stmt.c (working copy) > >> *************** gfc_trans_allocate (gfc_code * code) > >> *** 5200,5206 **** > >> } > >> /* else expr3 = NULL_TREE set above. */ > >> } > >> ! else > >> { > >> /* In all other cases evaluate the expr3 and create a > >> temporary. */ > >> --- 5200,5207 ---- > >> } > >> /* else expr3 = NULL_TREE set above. */ > >> } > >> ! else if (!(code->expr3->ts.type == BT_DERIVED > >> ! && code->expr3->ts.u.derived->attr.alloc_comp)) > >> { > >> /* In all other cases evaluate the expr3 and create a > >> temporary. */ > >> > >> When I get the code right, than all derived-typed source= expressions that > >> have an allocatable component will not be prepared for copy to the > >> allocated object. This also means, that functions returning an object of > >> such a type are called multiple times. Once for each object to allocate. > >> Is this really desired? > >> > >> I am sorry, that I have to say that, but the check2305.diff file does not > >> bring the testcase with it. > >> > >> Regards, > >> Andre > >> > >> > >> On Sat, 23 May 2015 14:48:53 +0200 > >> Paul Richard Thomas wrote: > >> > >>> Dear All, > >>> > >>> This patch started out fixing a single source of memory leak and then > >>> went on to fix various other issues that I found upon investigation. > >>> > >>> The fortran ChangeLog entry is sufficiently descripive that I do not > >>> think that there is a need to say more. > >>> > >>> Bootstrapped and regtested on x86_64/FC21 - OK for trunk? > >>> > >>> I am rather sure that some of the issues go further back than 6.0. I > >>> will investigate what should be fixed for 5.2. > >>> > >>> Cheers > >>> > >>> Paul > >>> > >>> 2015-05-23 Paul Thomas > >>> > >>> PR fortran/66079 > >>> * trans-expr.c (gfc_conv_procedure_call): Allocatable scalar > >>> function results must be freed and nullified after use. Create > >>> a temporary to hold the result to prevent duplicate calls. > >>> * trans-stmt.c (gfc_trans_allocate): Prevent memory leaks by > >>> not evaluating expr3 for scalar derived types with allocatable > >>> components. Fixed character length allocatable results and > >>> dummies need to be dereferenced. Also, if al_len is NULL use > >>> memsz for the string copy. > >>> > >>> 2015-05-23 Paul Thomas > >>> > >>> PR fortran/66079 > >>> * gfortran.dg/allocatable_scalar_13.f90: New test > >> > >> > >> -- > >> Andre Vehreschild * Email: vehre ad gmx dot de > > > > > > > > -- > > Outside of a dog, a book is a man's best friend. Inside of a dog it's > > too dark to read. > > > > Groucho Marx > > > -- Andre Vehreschild * Email: vehre ad gmx dot de --MP_/XnNfN5KmngRzakwjE.PUHA7 Content-Type: text/x-patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=pr66079.patch Content-length: 4081 diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c index 2eb297a..2ef9f15 100644 --- a/gcc/fortran/trans-stmt.c +++ b/gcc/fortran/trans-stmt.c @@ -5104,11 +5104,12 @@ gfc_trans_allocate (gfc_code * code) element size, i.e. _vptr%size, is stored in expr3_esize. Any of the trees may be the NULL_TREE indicating that this is not available for expr3's type. */ - tree expr3, expr3_vptr, expr3_len, expr3_esize; + tree expr3, expr3_vptr, expr3_len, expr3_esize, expr3_tmp_var; stmtblock_t block; stmtblock_t post; tree nelems; bool upoly_expr, tmp_expr3_len_flag = false, al_len_needs_set; + bool needs_comp_dealloc = false; gfc_symtree *newsym = NULL; if (!code->ext.alloc.list) @@ -5117,6 +5118,7 @@ gfc_trans_allocate (gfc_code * code) stat = tmp = memsz = al_vptr = al_len = NULL_TREE; expr3 = expr3_vptr = expr3_len = expr3_esize = NULL_TREE; label_errmsg = label_finish = errmsg = errlen = NULL_TREE; + expr3_tmp_var = NULL_TREE; gfc_init_block (&block); gfc_init_block (&post); @@ -5219,26 +5221,21 @@ gfc_trans_allocate (gfc_code * code) variable declaration. */ if (!VAR_P (se.expr)) { - tree var; tmp = build_fold_indirect_ref_loc (input_location, se.expr); /* We need a regular (non-UID) symbol here, therefore give a prefix. */ - var = gfc_create_var (TREE_TYPE (tmp), "expr3"); - gfc_add_modify_loc (input_location, &block, var, tmp); + expr3_tmp_var = gfc_create_var (TREE_TYPE (tmp), "source"); + gfc_add_modify_loc (input_location, &block, expr3_tmp_var, tmp); - /* Deallocate any allocatable components after all the allocations - and assignments of expr3 have been completed. */ - if (code->expr3->ts.type == BT_DERIVED + /* Check whether deallocate any allocatable components after + all the allocations and assignments of expr3 have been + completed are needed, in case we can't do move semantics. */ + needs_comp_dealloc = code->expr3->ts.type == BT_DERIVED && code->expr3->rank == 0 - && code->expr3->ts.u.derived->attr.alloc_comp) - { - tmp = gfc_deallocate_alloc_comp (code->expr3->ts.u.derived, - var, 0); - gfc_add_expr_to_block (&post, tmp); - } + && code->expr3->ts.u.derived->attr.alloc_comp; - tmp = var; + tmp = expr3_tmp_var; } else tmp = se.expr; @@ -5840,15 +5837,40 @@ gfc_trans_allocate (gfc_code * code) } else { - /* Switch off automatic reallocation since we have just + if (al->next == NULL && needs_comp_dealloc && expr->rank == 0) + { + /* Move the contents of expr3 into the last object to + allocate instead of allocating its allocatable components + anew. */ + tmp = gfc_build_memcpy_call (se.expr, expr3, + TYPE_SIZE_UNIT ( + TREE_TYPE (expr3))); + /* Because the allocatable components allocated for source= + are reused, do not add code to free them. */ + needs_comp_dealloc = false; + } + else + { + /* Switch off automatic reallocation since we have just done the ALLOCATE. */ - int realloc_lhs = flag_realloc_lhs; - flag_realloc_lhs = 0; - tmp = gfc_trans_assignment (gfc_expr_to_initialize (expr), - e3rhs, false, false); - flag_realloc_lhs = realloc_lhs; + int realloc_lhs = flag_realloc_lhs; + flag_realloc_lhs = 0; + tmp = gfc_trans_assignment (gfc_expr_to_initialize (expr), + e3rhs, false, false); + flag_realloc_lhs = realloc_lhs; + } } gfc_add_expr_to_block (&block, tmp); + + if (al->next == NULL && needs_comp_dealloc) + { + /* Allocatable components of expr3 are not "moved" into the + last object to allocate, but have to be freeed to prevent + memory loss. */ + tmp = gfc_deallocate_alloc_comp (code->expr3->ts.u.derived, + expr3_tmp_var, 0); + gfc_add_expr_to_block (&post, tmp); + } } else if (code->expr3 && code->expr3->mold && code->expr3->ts.type == BT_CLASS) --MP_/XnNfN5KmngRzakwjE.PUHA7--