libbacktrace patch committed: Call munmap after memory test

Message ID CAOyqgcWXq3iKd95kMY6r1NPWTZdq4mfPLcYHz7Jg8bATBsR-=Q@mail.gmail.com
State New
Headers show
Series
  • libbacktrace patch committed: Call munmap after memory test
Related show

Commit Message

Ian Lance Taylor April 17, 2018, 1:59 p.m.
The bug report https://github.com/ianlancetaylor/libbacktrace/issues/13
points out that when backtrace_full checks whether memory is
available, it doesn't necessarily release that memory.  It will stay
on the free list, so libbacktrace will use more and more memory over
time.  This patch fixes that problem by explicitly calling munmap.
Bootstrapped and ran libbacktrace and Go tests on x86_64-pc-linux-gnu.
Committed to mainline.

Ian


2018-04-17  Ian Lance Taylor  <iant@golang.org>

* backtrace.c (backtrace_full): When testing whether we can
allocate memory, call mmap directly, and munmap the memory.

Comments

Tom de Vries April 17, 2018, 5:21 p.m. | #1
On 04/17/2018 03:59 PM, Ian Lance Taylor wrote:
> The bug report https://github.com/ianlancetaylor/libbacktrace/issues/13

> points out that when backtrace_full checks whether memory is

> available, it doesn't necessarily release that memory.  It will stay

> on the free list, so libbacktrace will use more and more memory over

> time.  This patch fixes that problem by explicitly calling munmap.

> Bootstrapped and ran libbacktrace and Go tests on x86_64-pc-linux-gnu.

> Committed to mainline.

> 

> Ian

> 

> 

> 2018-04-17  Ian Lance Taylor  <iant@golang.org>

> 

> * backtrace.c (backtrace_full): When testing whether we can

> allocate memory, call mmap directly, and munmap the memory.

> 

> 

> patch.txt

> 

> 

> Index: backtrace.c

> ===================================================================

> --- backtrace.c	(revision 259359)

> +++ backtrace.c	(working copy)

> @@ -32,12 +32,26 @@ POSSIBILITY OF SUCH DAMAGE.  */

>   

>   #include "config.h"

>   

> +#include <unistd.h>

>   #include <sys/types.h>

>   

> +#if !BACKTRACE_USES_MALLOC

> +#include <sys/mman.h>

> +#endif

> +


Hi,

this breaks the nvptx build:
...
libbacktrace/backtrace.c:39:10: fatal error: sys/mman.h: No such file or 
directory
  #include <sys/mman.h>
           ^~~~~~~~~~~~
compilation terminated.
...

Thanks,
- Tom

>   #include "unwind.h"

>   #include "backtrace.h"

> +#include "backtrace-supported.h"

>   #include "internal.h"

>   

> +#ifndef MAP_ANONYMOUS

> +#define MAP_ANONYMOUS MAP_ANON

> +#endif

> +

> +#ifndef MAP_FAILED

> +#define MAP_FAILED ((void *)-1)

> +#endif

> +

>   /* The main backtrace_full routine.  */

>   

>   /* Data passed through _Unwind_Backtrace.  */

> @@ -104,7 +118,6 @@ backtrace_full (struct backtrace_state *

>   		backtrace_error_callback error_callback, void *data)

>   {

>     struct backtrace_data bdata;

> -  void *p;

>   

>     bdata.skip = skip + 1;

>     bdata.state = state;

> @@ -113,16 +126,25 @@ backtrace_full (struct backtrace_state *

>     bdata.data = data;

>     bdata.ret = 0;

>   

> -  /* If we can't allocate any memory at all, don't try to produce

> -     file/line information.  */

> -  p = backtrace_alloc (state, 4096, NULL, NULL);

> -  if (p == NULL)

> -    bdata.can_alloc = 0;

> -  else

> -    {

> -      backtrace_free (state, p, 4096, NULL, NULL);

> -      bdata.can_alloc = 1;

> -    }

> +#if !BACKTRACE_USES_MALLOC

> +  {

> +    size_t pagesize;

> +    void *page;

> +

> +    /* If we can't allocate any memory at all, don't try to produce

> +       file/line information.  */

> +    pagesize = getpagesize ();

> +    page = mmap (NULL, pagesize, PROT_READ | PROT_WRITE,

> +		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

> +    if (page == MAP_FAILED)

> +      bdata.can_alloc = 0;

> +    else

> +      {

> +	munmap (page, pagesize);

> +	bdata.can_alloc = 1;

> +      }

> +  }

> +#endif

>   

>     _Unwind_Backtrace (unwind, &bdata);

>     return bdata.ret;

>
Ian Lance Taylor April 17, 2018, 5:29 p.m. | #2
On Tue, Apr 17, 2018 at 10:21 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 04/17/2018 03:59 PM, Ian Lance Taylor wrote:

>>

>> The bug report https://github.com/ianlancetaylor/libbacktrace/issues/13

>> points out that when backtrace_full checks whether memory is

>> available, it doesn't necessarily release that memory.  It will stay

>> on the free list, so libbacktrace will use more and more memory over

>> time.  This patch fixes that problem by explicitly calling munmap.

>> Bootstrapped and ran libbacktrace and Go tests on x86_64-pc-linux-gnu.

>> Committed to mainline.

>>

>> Ian

>>

>>

>> 2018-04-17  Ian Lance Taylor  <iant@golang.org>

>>

>> * backtrace.c (backtrace_full): When testing whether we can

>> allocate memory, call mmap directly, and munmap the memory.

>>

>>

>> patch.txt

>>

>>

>> Index: backtrace.c

>> ===================================================================

>> --- backtrace.c (revision 259359)

>> +++ backtrace.c (working copy)

>> @@ -32,12 +32,26 @@ POSSIBILITY OF SUCH DAMAGE.  */

>>     #include "config.h"

>>   +#include <unistd.h>

>>   #include <sys/types.h>

>>   +#if !BACKTRACE_USES_MALLOC

>> +#include <sys/mman.h>

>> +#endif

>> +

>

>

> Hi,

>

> this breaks the nvptx build:

> ...

> libbacktrace/backtrace.c:39:10: fatal error: sys/mman.h: No such file or

> directory

>  #include <sys/mman.h>

>           ^~~~~~~~~~~~

> compilation terminated.


Sorry about that.  Committed this patch, which should fix that problem.

Ian


2018-04-17  Ian Lance Taylor  <iant@golang.org>

* backtrace.c: Include backtrace-supported.h before checking
BACKTRACE_USES_MALLOC.
Index: backtrace.c
===================================================================
--- backtrace.c	(revision 259434)
+++ backtrace.c	(working copy)
@@ -35,13 +35,14 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include <unistd.h>
 #include <sys/types.h>
 
+#include "backtrace-supported.h"
+
 #if !BACKTRACE_USES_MALLOC
 #include <sys/mman.h>
 #endif
 
 #include "unwind.h"
 #include "backtrace.h"
-#include "backtrace-supported.h"
 #include "internal.h"
 
 #ifndef MAP_ANONYMOUS
Ian Lance Taylor April 17, 2018, 5:58 p.m. | #3
On Tue, Apr 17, 2018 at 10:29 AM, Ian Lance Taylor <iant@golang.org> wrote:
> On Tue, Apr 17, 2018 at 10:21 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:

>> On 04/17/2018 03:59 PM, Ian Lance Taylor wrote:

>>>

>>> The bug report https://github.com/ianlancetaylor/libbacktrace/issues/13

>>> points out that when backtrace_full checks whether memory is

>>> available, it doesn't necessarily release that memory.  It will stay

>>> on the free list, so libbacktrace will use more and more memory over

>>> time.  This patch fixes that problem by explicitly calling munmap.

>>> Bootstrapped and ran libbacktrace and Go tests on x86_64-pc-linux-gnu.

>>> Committed to mainline.

>>>

>>> Ian

>>>

>>>

>>> 2018-04-17  Ian Lance Taylor  <iant@golang.org>

>>>

>>> * backtrace.c (backtrace_full): When testing whether we can

>>> allocate memory, call mmap directly, and munmap the memory.

>>>

>>>

>>> patch.txt

>>>

>>>

>>> Index: backtrace.c

>>> ===================================================================

>>> --- backtrace.c (revision 259359)

>>> +++ backtrace.c (working copy)

>>> @@ -32,12 +32,26 @@ POSSIBILITY OF SUCH DAMAGE.  */

>>>     #include "config.h"

>>>   +#include <unistd.h>

>>>   #include <sys/types.h>

>>>   +#if !BACKTRACE_USES_MALLOC

>>> +#include <sys/mman.h>

>>> +#endif

>>> +

>>

>>

>> Hi,

>>

>> this breaks the nvptx build:

>> ...

>> libbacktrace/backtrace.c:39:10: fatal error: sys/mman.h: No such file or

>> directory

>>  #include <sys/mman.h>

>>           ^~~~~~~~~~~~

>> compilation terminated.

>

> Sorry about that.  Committed this patch, which should fix that problem.


Actually, never mind.  Looks like this didn't fix the problem.  I'm
just reverting back to the state as of earlier today.

Ian

2018-04-17  Ian Lance Taylor  <iant@golang.org>

* backtrace.c: Revert last two changes.  Don't call mmap
directly.
Index: backtrace.c
===================================================================
--- backtrace.c	(revision 259439)
+++ backtrace.c	(working copy)
@@ -32,27 +32,12 @@ POSSIBILITY OF SUCH DAMAGE.  */
 
 #include "config.h"
 
-#include <unistd.h>
 #include <sys/types.h>
 
-#include "backtrace-supported.h"
-
-#if !BACKTRACE_USES_MALLOC
-#include <sys/mman.h>
-#endif
-
 #include "unwind.h"
 #include "backtrace.h"
 #include "internal.h"
 
-#ifndef MAP_ANONYMOUS
-#define MAP_ANONYMOUS MAP_ANON
-#endif
-
-#ifndef MAP_FAILED
-#define MAP_FAILED ((void *)-1)
-#endif
-
 /* The main backtrace_full routine.  */
 
 /* Data passed through _Unwind_Backtrace.  */
@@ -119,6 +104,7 @@ backtrace_full (struct backtrace_state *
 		backtrace_error_callback error_callback, void *data)
 {
   struct backtrace_data bdata;
+  void *p;
 
   bdata.skip = skip + 1;
   bdata.state = state;
@@ -127,25 +113,16 @@ backtrace_full (struct backtrace_state *
   bdata.data = data;
   bdata.ret = 0;
 
-#if !BACKTRACE_USES_MALLOC
-  {
-    size_t pagesize;
-    void *page;
-
-    /* If we can't allocate any memory at all, don't try to produce
-       file/line information.  */
-    pagesize = getpagesize ();
-    page = mmap (NULL, pagesize, PROT_READ | PROT_WRITE, 
-		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-    if (page == MAP_FAILED)
-      bdata.can_alloc = 0;
-    else
-      {
-	munmap (page, pagesize);
-	bdata.can_alloc = 1;
-      }
-  }
-#endif
+  /* If we can't allocate any memory at all, don't try to produce
+     file/line information.  */
+  p = backtrace_alloc (state, 4096, NULL, NULL);
+  if (p == NULL)
+    bdata.can_alloc = 0;
+  else
+    {
+      backtrace_free (state, p, 4096, NULL, NULL);
+      bdata.can_alloc = 1;
+    }
 
   _Unwind_Backtrace (unwind, &bdata);
   return bdata.ret;

Patch

Index: backtrace.c
===================================================================
--- backtrace.c	(revision 259359)
+++ backtrace.c	(working copy)
@@ -32,12 +32,26 @@  POSSIBILITY OF SUCH DAMAGE.  */
 
 #include "config.h"
 
+#include <unistd.h>
 #include <sys/types.h>
 
+#if !BACKTRACE_USES_MALLOC
+#include <sys/mman.h>
+#endif
+
 #include "unwind.h"
 #include "backtrace.h"
+#include "backtrace-supported.h"
 #include "internal.h"
 
+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+
+#ifndef MAP_FAILED
+#define MAP_FAILED ((void *)-1)
+#endif
+
 /* The main backtrace_full routine.  */
 
 /* Data passed through _Unwind_Backtrace.  */
@@ -104,7 +118,6 @@  backtrace_full (struct backtrace_state *
 		backtrace_error_callback error_callback, void *data)
 {
   struct backtrace_data bdata;
-  void *p;
 
   bdata.skip = skip + 1;
   bdata.state = state;
@@ -113,16 +126,25 @@  backtrace_full (struct backtrace_state *
   bdata.data = data;
   bdata.ret = 0;
 
-  /* If we can't allocate any memory at all, don't try to produce
-     file/line information.  */
-  p = backtrace_alloc (state, 4096, NULL, NULL);
-  if (p == NULL)
-    bdata.can_alloc = 0;
-  else
-    {
-      backtrace_free (state, p, 4096, NULL, NULL);
-      bdata.can_alloc = 1;
-    }
+#if !BACKTRACE_USES_MALLOC
+  {
+    size_t pagesize;
+    void *page;
+
+    /* If we can't allocate any memory at all, don't try to produce
+       file/line information.  */
+    pagesize = getpagesize ();
+    page = mmap (NULL, pagesize, PROT_READ | PROT_WRITE, 
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (page == MAP_FAILED)
+      bdata.can_alloc = 0;
+    else
+      {
+	munmap (page, pagesize);
+	bdata.can_alloc = 1;
+      }
+  }
+#endif
 
   _Unwind_Backtrace (unwind, &bdata);
   return bdata.ret;