Re: [Patch, fortran] PR66079 - [6 Regression] memory leak with source allocation in internal subprogram

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Dear Andre,

I am perfectly happy with renaming the rename to "source". I was
attempting to distinguish "atmp" coming from trans-array.c from this
temporary; just as an aid to any possible future debugging.

The rework of the patch looks fine to me as well. Do you want to
commit or should I do so?

Cheers

Paul

On 25 May 2015 at 12:24, Andre Vehreschild <vehre@gmx.de> wrote:
> 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 <paul.richard.thomas@gmail.com> 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  <pault@gcc.gnu.org>
>>
>>     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  <pault@gcc.gnu.org>
>>
>>     PR fortran/66079
>>     * gfortran.dg/allocatable_scalar_13.f90: New test
>>
>>
>> On 24 May 2015 at 09:51, Paul Richard Thomas
>> <paul.richard.thomas@gmail.com> 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 <vehre@gmx.de> 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 <paul.richard.thomas@gmail.com> 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  <pault@gcc.gnu.org>
>> >>>
>> >>>     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  <pault@gcc.gnu.org>
>> >>>
>> >>>     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



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx