[m-rev.] for post-commit review: fix bug #243: memory zone leak on Windows
Julien Fischer
juliensf at csse.unimelb.edu.au
Wed Dec 7 04:54:36 AEDT 2011
For post-commit review by Ian.
(Peter Ross might also want to have a look at this one.)
Ian, this differs a little bit from the diff I sent you the other day.
(The changes are related to MSVC stuff.)
I've tested this with both GCC and MSVC and it works with both of them.
Branches: main, 11.07
Fix bug #243: the management of memory zones on Windows was broken, causing a
memory leak in trseg and (presumably) stseg grades. In grades that use
conservative GC on most systems (e.g. Linux or Mac OS X), allocation of memory
for memory zones is handled by the Boehm collector. On Windows, the situation
was a bit different because we were trying to emulate mprotect style memory
protection. Instead of allocating the memory for the zone using the Boehm
collector, the memory was reserved using a call to VirtualAlloc and then all
the memory within the zone was added to the Boehm GC's root set. We could
then use VirtualAlloc and friends to emulate mprotect. In principle, this
is fine, in practice there are number of problems with it.
When a memory zone was deallocated the call to VirtualFree was silently failing
(the return value of this call wasn't checked) -- this meant that the memory in
the zone was not being freed, hence the leak. The immediate cause of this is
that we are calling VirtualFree with the wrong argument values for those used
in the corresponding call to VirtulAlloc. Unfortunately, putting the "correct"
value in, just results in a segmentation fault. (I suspect we haven't actually
been allocating the memory correctly either.)
Aside from this a second problem is that while we register the words in the
memory zone as GC roots when we allocate it, we do not remove them from the
GC root set when we deallocate the zone. One rather obvious reason for
this is that the Boehm GC doesn't support doing this on Windows!
Yet another issue with all this is that while MR_protect_pages calls
VirtualAlloc, the documentation in the code suggests it should be calling
VirtualProtect.
The fix here is to replace the VirtualAlloc way of managing memory zones on
Windows with the Boehm GC approach used on all the other systems. The former
is pretty much completely broken and only appeared to work because most grades
only allocate a few memory zones at program startup and never deallocate them.
This does mean the mprotect emulation we were doing on Windows will no longer
work, but that's the lesser of two evils. (It may be possible to reinstate
something like it using VirtualProtect, but that will require looking carefully
at exactly how the Boehm collector allocates memory on Windows.)
runtime/mercury_memory_zones.c:
Use the Boehm GC for allocating memory zones on Windows when
in grade that uses conservative GC.
Delete the flawed approach that uses VirtualAlloc.
runtime/mercury_conf_param.h:
Do not define the macro MR_WIN32_VIRTUAL_ALLOC on Windows: we no
longer use VirtualAlloc anywhere.
Do not define MR_PROTECTPAGE and MR_CHECK_OVERFLOW_VIA_MPROTECT
on Windows, since we no longer do any mprotect emulation on
Windows.
runtime/mercury_memory_handlers.c:
Properly protect some code in the handler for Windows memory
access errors that assumes that memory zones have a redzone.
(After this change they won't have a redzone.)
Julien.
Index: runtime/mercury_conf_param.h
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_conf_param.h,v
retrieving revision 1.123
diff -u -r1.123 mercury_conf_param.h
--- runtime/mercury_conf_param.h 5 Nov 2011 15:58:16 -0000 1.123
+++ runtime/mercury_conf_param.h 6 Dec 2011 17:18:01 -0000
@@ -41,15 +41,12 @@
**
** MR_WIN32_GETSYSTEMINFO -- Is GetSystemInfo() available?
**
-** MR_WIN32_VIRTUAL_ALLOC -- Is VirtualAlloc() available?
-**
** MR_BROKEN_ST_INO - Is the st_ino field of `struct stat' junk.
** Windows doesn't fill in this field correctly.
*/
#ifdef _WIN32
#define MR_WIN32
#define MR_WIN32_GETSYSTEMINFO
- #define MR_WIN32_VIRTUAL_ALLOC
#define MR_WIN32_GETPROCESSTIMES
#define MR_BROKEN_ST_INO
#endif
@@ -1054,7 +1051,7 @@
** memory zones using mprotect() like
** functionality.
*/
-#if (defined(MR_HAVE_MPROTECT) && defined(MR_HAVE_SIGINFO)) || defined(_WIN32)
+#if (defined(MR_HAVE_MPROTECT) && defined(MR_HAVE_SIGINFO))
#define MR_CHECK_OVERFLOW_VIA_MPROTECT
#endif
@@ -1062,7 +1059,7 @@
** MR_PROTECTPAGE -- MR_protect_pages() can be defined to provide the same
** functionality as the system call mprotect().
*/
-#if defined(MR_HAVE_MPROTECT) || defined(_WIN32)
+#if defined(MR_HAVE_MPROTECT)
#define MR_PROTECTPAGE
#endif
Index: runtime/mercury_memory_handlers.c
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_memory_handlers.c,v
retrieving revision 1.36
diff -u -r1.36 mercury_memory_handlers.c
--- runtime/mercury_memory_handlers.c 16 Jul 2011 07:51:30 -0000 1.36
+++ runtime/mercury_memory_handlers.c 6 Dec 2011 17:18:02 -0000
@@ -124,7 +124,7 @@
static MR_bool
MR_try_munprotect(void *addr, void *context)
{
-#if !(defined(MR_HAVE_SIGINFO) || defined(MR_WIN32_VIRTUAL_ALLOC))
+#if !defined(MR_HAVE_SIGINFO)
return MR_FALSE;
#else
MR_Word *fault_addr;
@@ -706,6 +706,7 @@
access_mode);
}
+ #if defined(MR_CHECK_OVERFLOW_VIA_MPROTECT)
fprintf(stderr, "\n*** Trying to see if this "
"stands within a mercury zone...");
/*
@@ -748,6 +749,7 @@
zone, rec); */
zone = zone->MR_zone_next;
}
+ #endif /* MR_CHECK_OVERFLOW_VIA_MPROTECT */
}
return;
}
Index: runtime/mercury_memory_zones.c
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_memory_zones.c,v
retrieving revision 1.44
diff -u -r1.44 mercury_memory_zones.c
--- runtime/mercury_memory_zones.c 23 Nov 2011 10:53:01 -0000 1.44
+++ runtime/mercury_memory_zones.c 6 Dec 2011 17:18:03 -0000
@@ -69,14 +69,6 @@
#include "mercury_memory_handlers.h"
/*
-** XXX: Why is this included here and not above with the other system
-** includes?
-*/
-#ifdef MR_WIN32_VIRTUAL_ALLOC
- #include "mercury_windows.h"
-#endif
-
-/*
** This macro can be used to update a high water mark of a statistic.
*/
#define MR_UPDATE_HIGHWATER(max, cur) \
@@ -124,95 +116,11 @@
{
return mprotect((char *) addr, size, prot_flags);
}
-
-#elif defined(MR_WIN32_VIRTUAL_ALLOC)
-
-/*
-** Emulate mprotect under Win32.
-** Return -1 on failure
-*/
-
-int
-MR_protect_pages(void *addr, size_t size, int prot_flags)
-{
- int rc;
- DWORD flags;
-
- if (prot_flags & PROT_WRITE) {
- flags = PAGE_READWRITE;
- } else if (prot_flags & PROT_READ) {
- flags = PAGE_READONLY;
- } else {
- flags = PAGE_NOACCESS;
- }
-
- rc = (VirtualAlloc(addr, size, MEM_COMMIT, flags) ? 0 : -1);
- if (rc < 0) {
- fprintf(stderr,
- "Error in VirtualAlloc(addr=0x%08lx, size=0x%08lx): 0x%08lx\n",
- (unsigned long) addr, (unsigned long) size,
- (unsigned long) GetLastError());
- }
-
- return rc;
-}
-
-#endif /* MR_WIN32_VIRTUAL_ALLOC */
+#endif
/*---------------------------------------------------------------------------*/
-#if defined(MR_WIN32_VIRTUAL_ALLOC)
-
-/*
-** Under Win32, we use VirtualAlloc instead of the standard malloc,
-** since we will have to call VirtualProtect later on the pages
-** allocated here.
-*/
-
-static void *
-MR_alloc_zone_memory(size_t size)
-{
- void *ptr;
-
- ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
- if (ptr == NULL) {
- fprintf(stderr, "Error in VirtualAlloc(size=0x%08lx): 0x%08lx\n",
- (unsigned long) size, (unsigned long) GetLastError());
- }
-
- #if defined(MR_HGC)
- if (ptr != NULL) {
- MR_hgc_add_roots_range((char *) ptr, ((char *) ptr) + size);
- }
- #elif defined(MR_CONSERVATIVE_GC)
- if (ptr != NULL) {
- GC_add_roots((char *) ptr, (char *) ptr + size);
- }
- #endif
- return ptr;
-}
-
-static void *
-MR_realloc_zone_memory(void *old_base, size_t copy_size, size_t new_size)
-{
- void *ptr;
-
- ptr = MR_alloc_zone_memory(new_size);
- (void) MR_memcpy(ptr, old_base, copy_size);
- /*
- ** XXX We should of course release old_base's memory, but I don't know
- ** enough about Windows even to implement that. -zs
- */
- return ptr;
-}
-
-static void
-MR_dealloc_zone_memory(void *base, size_t size)
-{
- VirtualFree(base, size, MEM_RELEASE);
-}
-
-#elif defined(MR_CONSERVATIVE_GC)
+#if defined(MR_CONSERVATIVE_GC)
static void *
MR_alloc_zone_memory(size_t size)
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to: mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions: mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------
More information about the reviews
mailing list