[PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

The uses of MPFR in gimple-ssa-sprintf.c apparently cause
the library to allocates some internal caches that it then leaks
on program exit, causing Valgrind memory leak errors.  The MPFR
manual "strongly advises to [call mpfr_free_cache] before
terminating a thread, or before exiting when using tools like
'valgrind' (to avoid memory leaks being reported)) so the attached
patch does just that.

It' seems like an obvious fix that could presumably be committed
without a review or approval but I'd like to give others a chance
to comment on the placement of the call and whether it should be
guarded by ENABLE_VALGRIND_ANNOTATIONS.

Joseph, since you commented on the bug, do you have a suggestion
for a different site for it or a guard?  The only other call to
the function is in the Fortran FE and it's neither guarded nor
does it appear to ever be called.

Thanks
Martin

[-- Attachment #2: gcc-79699.diff --]
[-- Type: text/x-patch, Size: 592 bytes --]

PR tree-optimization/79699 - small memory leak in MPFR

gcc/ChangeLog:

	PR tree-optimization/79699
	* gimple-ssa-sprintf.c (pass_sprintf_length::execute): Free MPFR
	caches to avoid a memory leak on program exit.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 7688439..0c00fa0 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -3612,6 +3612,8 @@ pass_sprintf_length::execute (function *fun)
   /* Clean up object size info.  */
   fini_object_sizes ();
 
+  /* Clean up MPFR caches (see bug 79699).  */
+  mpfr_free_cache ();
   return 0;
 }
 

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Joseph Myers]

On Wed, 1 Mar 2017, Martin Sebor wrote:

> Joseph, since you commented on the bug, do you have a suggestion
> for a different site for it or a guard?  The only other call to
> the function is in the Fortran FE and it's neither guarded nor
> does it appear to ever be called.

I don't think a guard is needed.  Arguably it should be called from an 
atexit handler, but since we don't have such a handler calling it from the 
relevant pass seems reasonable (and I'm not sure what the right way to 
handle such freeing of memory in the JIT context is).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Richard Biener]

On Thu, Mar 2, 2017 at 2:01 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 1 Mar 2017, Martin Sebor wrote:
>
>> Joseph, since you commented on the bug, do you have a suggestion
>> for a different site for it or a guard?  The only other call to
>> the function is in the Fortran FE and it's neither guarded nor
>> does it appear to ever be called.
>
> I don't think a guard is needed.  Arguably it should be called from an
> atexit handler, but since we don't have such a handler calling it from the
> relevant pass seems reasonable (and I'm not sure what the right way to
> handle such freeing of memory in the JIT context is).

IMHO we should call it from gcc::~context

Richard.

> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On 03/02/2017 01:08 AM, Richard Biener wrote:
> On Thu, Mar 2, 2017 at 2:01 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Wed, 1 Mar 2017, Martin Sebor wrote:
>>
>>> Joseph, since you commented on the bug, do you have a suggestion
>>> for a different site for it or a guard?  The only other call to
>>> the function is in the Fortran FE and it's neither guarded nor
>>> does it appear to ever be called.
>>
>> I don't think a guard is needed.  Arguably it should be called from an
>> atexit handler, but since we don't have such a handler calling it from the
>> relevant pass seems reasonable (and I'm not sure what the right way to
>> handle such freeing of memory in the JIT context is).
>
> IMHO we should call it from gcc::~context

Thanks, that makes sense to me.  The attached patch does that.

Martin

[-- Attachment #2: gcc-79699.diff --]
[-- Type: text/x-patch, Size: 707 bytes --]

PR tree-optimization/79699 - small memory leak in MPFR

gcc/ChangeLog:

	PR tree-optimization/79699
	* context.c (context::~context): Free MPFR caches to avoid
	a memory leak on program exit.

diff --git a/gcc/context.c b/gcc/context.c
index a7ded9c..d2009b9 100644
--- a/gcc/context.c
+++ b/gcc/context.c
@@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "context.h"
 #include "pass_manager.h"
 #include "dumpfile.h"
+#include "mpfr.h"
 
 /* The singleton holder of global state: */
 gcc::context *g;
@@ -42,4 +43,7 @@ gcc::context::~context ()
 {
   delete m_passes;
   delete m_dumps;
+
+  /* Release MPFR caches to avoid Valgrind leak reports.  */
+  mpfr_free_cache ();
 }

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Andrew Pinski]

On Thu, Mar 2, 2017 at 1:33 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 03/02/2017 01:08 AM, Richard Biener wrote:
>>
>> On Thu, Mar 2, 2017 at 2:01 AM, Joseph Myers <joseph@codesourcery.com>
>> wrote:
>>>
>>> On Wed, 1 Mar 2017, Martin Sebor wrote:
>>>
>>>> Joseph, since you commented on the bug, do you have a suggestion
>>>> for a different site for it or a guard?  The only other call to
>>>> the function is in the Fortran FE and it's neither guarded nor
>>>> does it appear to ever be called.
>>>
>>>
>>> I don't think a guard is needed.  Arguably it should be called from an
>>> atexit handler, but since we don't have such a handler calling it from
>>> the
>>> relevant pass seems reasonable (and I'm not sure what the right way to
>>> handle such freeing of memory in the JIT context is).
>>
>>
>> IMHO we should call it from gcc::~context
>
>
> Thanks, that makes sense to me.  The attached patch does that.

Is this function call thread safe?  Or rather is MPFR thread safe?
I am thinking of the case where there are two gcc::context around one
running in each thread.  I am not saying this is the current behavior
but I do know there was talk about making GCC thread safe before and I
want to make sure we understand that this might cause issues in that
goal.

Thanks,
Andrew

>
> Martin

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Martin Sebor]

On 03/02/2017 02:40 PM, Andrew Pinski wrote:
> On Thu, Mar 2, 2017 at 1:33 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 03/02/2017 01:08 AM, Richard Biener wrote:
>>>
>>> On Thu, Mar 2, 2017 at 2:01 AM, Joseph Myers <joseph@codesourcery.com>
>>> wrote:
>>>>
>>>> On Wed, 1 Mar 2017, Martin Sebor wrote:
>>>>
>>>>> Joseph, since you commented on the bug, do you have a suggestion
>>>>> for a different site for it or a guard?  The only other call to
>>>>> the function is in the Fortran FE and it's neither guarded nor
>>>>> does it appear to ever be called.
>>>>
>>>>
>>>> I don't think a guard is needed.  Arguably it should be called from an
>>>> atexit handler, but since we don't have such a handler calling it from
>>>> the
>>>> relevant pass seems reasonable (and I'm not sure what the right way to
>>>> handle such freeing of memory in the JIT context is).
>>>
>>>
>>> IMHO we should call it from gcc::~context
>>
>>
>> Thanks, that makes sense to me.  The attached patch does that.
>
> Is this function call thread safe?  Or rather is MPFR thread safe?
> I am thinking of the case where there are two gcc::context around one
> running in each thread.  I am not saying this is the current behavior
> but I do know there was talk about making GCC thread safe before and I
> want to make sure we understand that this might cause issues in that
> goal.

The latest MPFR manual implies the function is thread safe if
MPFR itself has been built as thread safe:

   http://www.mpfr.org/mpfr-current/mpfr.html#Memory-Handling-1

Similar text is in the MPFR 2.4 manual (and so is mpfr_free_cache)
so it should be safe to call the function even with older versions
of the library.

Martin

Re: [PATCH] free MPFR caches in gimple-ssa-sprintf.c (PR 79699)

[Richard Biener]

On Thu, Mar 2, 2017 at 10:33 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 03/02/2017 01:08 AM, Richard Biener wrote:
>>
>> On Thu, Mar 2, 2017 at 2:01 AM, Joseph Myers <joseph@codesourcery.com>
>> wrote:
>>>
>>> On Wed, 1 Mar 2017, Martin Sebor wrote:
>>>
>>>> Joseph, since you commented on the bug, do you have a suggestion
>>>> for a different site for it or a guard?  The only other call to
>>>> the function is in the Fortran FE and it's neither guarded nor
>>>> does it appear to ever be called.
>>>
>>>
>>> I don't think a guard is needed.  Arguably it should be called from an
>>> atexit handler, but since we don't have such a handler calling it from
>>> the
>>> relevant pass seems reasonable (and I'm not sure what the right way to
>>> handle such freeing of memory in the JIT context is).
>>
>>
>> IMHO we should call it from gcc::~context
>
>
> Thanks, that makes sense to me.  The attached patch does that.

"mpfr.h" as include is wrong, please use "realmpfr.h" as all other code in GCC
(which includes <mpfr.h> and <mpc.h>, ideally those would move to system.h
guarded with a INCLUDE_XXX guard or always included as we include gmp.h).

Ok with that change,
Richard.

> Martin