From 9bd83e82924b7f703e8298e631004d44d5c27d8f Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Fri, 23 Mar 2018 14:33:15 +0000 Subject: [PATCH] Add memory debugging This adds a new configure option to GAP which enables an extremely slow debug mode, which can find problems where references to memory are incorrectly kept across garbage collections. --- configure.ac | 8 +++ lib/init.g | 3 + lib/system.g | 3 +- src/gap.c | 27 ++++++++- src/gasman.c | 122 ++++++++++++++++++++++++++++++++++++- src/gasman.h | 20 ++++--- src/system.c | 166 +++++++++++++++++++++++++++++++++++++++++---------- 7 files changed, 306 insertions(+), 43 deletions(-) diff --git a/configure.ac b/configure.ac index 625defb17f6..4a7f419882e 100644 --- a/configure.ac +++ b/configure.ac @@ -207,6 +207,14 @@ AC_ARG_ENABLE([debug], AC_MSG_CHECKING([whether to enable debug mode]) AC_MSG_RESULT([$enable_debug]) +AC_ARG_ENABLE([memory-checking], + [AS_HELP_STRING([--enable-memory-checking], [enable memory checking])], + [AC_DEFINE([GAP_MEM_CHECK], [1], [define if building with memory checking])], + [enable_memory_checking=no] + ) +AC_MSG_CHECKING([whether to enable memory checking]) +AC_MSG_RESULT([$enable_memory_checking]) + dnl dnl User setting: Enable -Werror (off by default) dnl diff --git a/lib/init.g b/lib/init.g index c42cb0685de..168b8a36ef0 100644 --- a/lib/init.g +++ b/lib/init.g @@ -560,6 +560,9 @@ BindGlobal( "ShowKernelInformation", function() if GAPInfo.KernelInfo.KernelDebug then Add( config, "KernelDebug" ); fi; + if GAPInfo.KernelInfo.MemCheck then + Add(config, "MemCheck (EXPERIMENTAL)"); + fi; if config <> [] then print_info( " Configuration: ", config, "\n" ); fi; diff --git a/lib/system.g b/lib/system.g index ca0cbe72a2b..99e8ee4dfd3 100644 --- a/lib/system.g +++ b/lib/system.g @@ -110,7 +110,8 @@ BIND_GLOBAL( "GAPInfo", rec( help := [ "Run ProfileLineByLine() with recordMem := true on GAP start"] ), rec( long := "cover", default := "", arg := "", help := [ "Run CoverageLineByLine() on GAP start"] ), - ], + rec( long := "enableMemCheck", default := false) + ], ) ); diff --git a/src/gap.c b/src/gap.c index 1a55677ad5f..49722de9cb3 100644 --- a/src/gap.c +++ b/src/gap.c @@ -1880,6 +1880,18 @@ Obj FuncGASMAN_LIMITS( Obj self ) return list; } +#ifdef GAP_MEM_CHECK + +extern Int EnableMemCheck; + +Obj FuncGASMAN_MEM_CHECK(Obj self, Obj newval) +{ + EnableMemCheck = INT_INTOBJ(newval); + return True; +} + +#endif + /**************************************************************************** ** *F FuncTotalMemoryAllocated( ) .expert function 'TotalMemoryAllocated' @@ -2672,6 +2684,13 @@ Obj FuncKERNEL_INFO(Obj self) { AssPRec(res, r, False); #endif + r = RNamName("MemCheck"); +#ifdef GAP_MEM_CHECK + AssPRec(res, r, True); +#else + AssPRec(res, r, False); +#endif + MakeImmutable(res); return res; @@ -2767,7 +2786,7 @@ void ThreadedInterpreter(void *funcargs) { ** *V GVarFuncs . . . . . . . . . . . . . . . . . . list of functions to export */ -static StructGVarFunc GVarFuncs [] = { +static StructGVarFunc GVarFuncs[] = { GVAR_FUNC(Runtime, 0, ""), GVAR_FUNC(RUNTIMES, 0, ""), @@ -2788,6 +2807,9 @@ static StructGVarFunc GVarFuncs [] = { GVAR_FUNC(GASMAN_STATS, 0, ""), GVAR_FUNC(GASMAN_MESSAGE_STATUS, 0, ""), GVAR_FUNC(GASMAN_LIMITS, 0, ""), +#ifdef GAP_MEM_CHECK + GVAR_FUNC(GASMAN_MEM_CHECK, 1, "int"), +#endif GVAR_FUNC(TotalMemoryAllocated, 0, ""), GVAR_FUNC(SIZE_OBJ, 1, "object"), GVAR_FUNC(TNUM_OBJ, 1, "object"), @@ -2802,7 +2824,8 @@ static StructGVarFunc GVarFuncs [] = { GVAR_FUNC(QUIT_GAP, -1, "args"), GVAR_FUNC(FORCE_QUIT_GAP, -1, "args"), GVAR_FUNC(SHOULD_QUIT_ON_BREAK, 0, ""), - GVAR_FUNC(SHELL, -1, "context, canReturnVoid, canReturnObj, lastDepth, setTime, prompt, promptHook, infile, outfile"), + GVAR_FUNC(SHELL, -1, "context, canReturnVoid, canReturnObj, lastDepth, " + "setTime, prompt, promptHook, infile, outfile"), GVAR_FUNC(CALL_WITH_CATCH, 2, "func, args"), GVAR_FUNC(JUMP_TO_CATCH, 1, "payload"), GVAR_FUNC(KERNEL_INFO, 0, ""), diff --git a/src/gasman.c b/src/gasman.c index 5368463478d..3e097e64089 100644 --- a/src/gasman.c +++ b/src/gasman.c @@ -119,6 +119,10 @@ #include +#ifdef GAP_MEM_CHECK +#include +#endif + /**************************************************************************** ** *F WORDS_BAG( ) . . . . . . . . . . words used by a bag of given size @@ -973,6 +977,113 @@ void InitCollectFuncBags ( AfterCollectFuncBags = after_func; } +/*************************************************************** + * GAP_MEM_CHECK + * + * One of the hardest categories of bugs to fix in GAP are where + * a reference to the internals of a GAP object are kept across + * a garbage collection (which moves GAP objects around). + * + * MultiHeap provides a method of detecting such problems, at + * the cost of GREATLY decreased performance (Starting GAP in + * MultiHeap mode takes hours, rather than seconds). + * + * The fundamental idea behind MultiHeap is, whenever NewBag + * or ResizeBag is called, then the contents of every Bag in + * GAP is moved, and the memory previously being used is marked + * as not readable or writable using 'mprotect'. + * + * Actually copying all GAP's memory space would be extremely + * expensive, so instead we use 'mmap' to set up a set of copies + * of the GAP memory space, which are represented by the same + * underlying physical memory. + * + * The 0th such copy (which we also ensure is the one at the + * lowest memory address) is special -- this is where we + * reference the Master Pointers (which can't move). We do not + * 'mprotect' any of this memory. + * + * Every time we call 'NewBag' or 'ResizeBag', we change which + * copy of the GASMAN memory space the master pointers point + * to, disabling access to the previous copy, and enabling access + * to the new one. + * + * We never use the master pointers in any copy other than the + * 0th, and we never refer to the Bag Area in the 0th copy. However, + * it simplifies things to not try to seperate the master pointer + * and Bag areas, because the master pointer area can grow as GAP + * runs. + * + * Because this code is VERY slow, it can be turned on and off. + * At run time, call GASMAN_MEM_CHECK(1) to enable, and + * GASMAN_MEM_CHECK(0) to disable. Start GAP with --enableMemCheck + * to enable from GAP's start + */ + +#ifdef GAP_MEM_CHECK + +Int EnableMemCheck = 0; + +Int enableMemCheck(Char ** argv, void * dummy) +{ + EnableMemCheck = 1; + return 1; +} + +void MoveBagMemory(char * oldbase, char * newbase) +{ + Int moveSize = (newbase - oldbase) / sizeof(Bag); + /* update the masterpointers */ + for (Bag * p = MptrBags; p < MptrEndBags; p++) { + if ((Bag)MptrEndBags <= *p) + *p += moveSize; + } + + /* update 'OldBags', 'YoungBags', 'AllocBags', and 'EndBags' */ + OldBags += moveSize; + YoungBags += moveSize; + AllocBags += moveSize; + EndBags += moveSize; +} + + +// From system.c + +UInt GetMembufCount(void); +void * GetMembuf(UInt i); +UInt GetMembufSize(void); + +void MaybeMoveBags(void) +{ + static Int oldBase = 0; + + if (!EnableMemCheck) + return; + + Int newBase = oldBase + 1; + // 0 is special, skip it when cycling + if (newBase == GetMembufCount()) + newBase = 1; + + mprotect(GetMembuf(newBase), GetMembufSize(), PROT_READ | PROT_WRITE); + + /* call the before function (if any) */ + if (BeforeCollectFuncBags != 0) + (*BeforeCollectFuncBags)(); + + MoveBagMemory(GetMembuf(oldBase), GetMembuf(newBase)); + + /* call the after function (if any) */ + if (AfterCollectFuncBags != 0) + (*AfterCollectFuncBags)(); + + if (oldBase != 0) { + mprotect(GetMembuf(oldBase), GetMembufSize(), PROT_NONE); + } + + oldBase = newBase; +} +#endif /**************************************************************************** ** @@ -1104,6 +1215,10 @@ Bag NewBag ( { Bag bag; /* identifier of the new bag */ +#ifdef GAP_MEM_CHECK + MaybeMoveBags(); +#endif + #ifdef TREMBLE_HEAP CollectBags(0,0); #endif @@ -1269,6 +1384,11 @@ UInt ResizeBag ( Bag bag, UInt new_size ) { + +#ifdef GAP_MEM_CHECK + MaybeMoveBags(); +#endif + #ifdef TREMBLE_HEAP CollectBags(0,0); #endif @@ -2011,7 +2131,6 @@ UInt CollectBags ( else { done = 1; } - } /* if we already performed a full garbage collection */ @@ -2135,7 +2254,6 @@ UInt CollectBags ( return 1; } - /**************************************************************************** ** *F CheckMasterPointers() . . . . do consistency checks on the masterpointers diff --git a/src/gasman.h b/src/gasman.h index 307162992ce..0faba93756b 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -594,11 +594,11 @@ extern void SwapMasterPoint ( ** message. ** ** 'NrHalfDeadBags' -** +** ** 'NrHalfDeadBags' is the number of bags that have been found to be ** reachable only by way of weak pointers since the last garbage collection. ** The bodies of these bags are deleted, but their identifiers are marked so -** that weak pointer objects can recognize this situation. +** that weak pointer objects can recognize this situation. */ extern UInt NrAllBags; @@ -894,7 +894,7 @@ extern Bag * AllocBags; GC_unregister_disappearing_link((void **)(loc)) #endif - + /**************************************************************************** ** *F InitSweepFuncBags(,) . . . . install sweeping function @@ -902,7 +902,7 @@ extern Bag * AllocBags; ** 'InitSweepFuncBags( , )' ** ** 'InitSweepFuncBags' installs the function as sweeping -** function for bags of type . +** function for bags of type . ** ** A sweeping function is a function that takes two arguments src and dst of ** type Bag *, and a third, length of type UInt, and returns nothing. When @@ -913,10 +913,10 @@ extern Bag * AllocBags; ** Those functions are applied during the garbage collection to each marked ** bag, i.e., bags that are assumed to be still live to move them to their ** new position. The intended use is for weak pointer bags, which must -** remove references to identifiers of any half-dead objects. +** remove references to identifiers of any half-dead objects. ** ** If no function is installed for a TNum, then the data is simply copied -** unchanged and this is done particularly quickly +** unchanged and this is done particularly quickly */ typedef void (* TNumSweepFuncBags ) ( @@ -927,7 +927,7 @@ typedef void (* TNumSweepFuncBags ) ( extern void InitSweepFuncBags ( UInt tnum, TNumSweepFuncBags sweep_func ); - + /**************************************************************************** ** @@ -1133,7 +1133,7 @@ extern void FinishBags( void ); ** This calls a C function on every bag, including ones that are not ** reachable from the root, and will be deleted at the next garbage ** collection, by simply walking the masterpointer area. Not terribly safe -** +** */ extern void CallbackForAllBags( @@ -1144,4 +1144,8 @@ extern void CallbackForAllBags( void *AllocateMemoryBlock(UInt size); #endif +#ifdef GAP_MEM_CHECK +Int enableMemCheck(Char ** argv, void * dummy); +#endif + #endif // GAP_GASMAN_H diff --git a/src/system.c b/src/system.c index 6a584546919..ffff88fc7ba 100644 --- a/src/system.c +++ b/src/system.c @@ -739,6 +739,97 @@ void * POOL = NULL; UInt * * * syWorkspace = NULL; UInt syWorksize = 0; +#ifdef GAP_MEM_CHECK + +#if !defined(HAVE_MADVISE) || !defined(SYS_IS_64_BIT) +#error MultiHeap requires MADVISE and 64-bit OS +#endif + + +/*************************************************************** + * GAP_MEM_CHECK + * + * The following code is used by GAP_MEM_CHECK support, which is + * documented in gasman.c + */ + +void SyMAdviseFree(void) +{ +} + + +enum { membufcount = 64 }; +static void * membufs[membufcount]; +static UInt membufSize; + +UInt GetMembufCount(void) +{ + return membufcount; +} + +void * GetMembuf(UInt i) +{ + return membufs[i]; +} + +UInt GetMembufSize(void) +{ + return membufSize; +} + +static int order_pointers(const void * a, const void * b) +{ + void * const * pa = a; + void * const * pb = b; + if (*pa < *pb) { + return -1; + } + else if (*pa > *pb) { + return 1; + } + else { + return 0; + } +} + +void * SyAnonMMap(size_t size) +{ + size = SyRoundUpToPagesize(size); + membufSize = size; + + unlink("/dev/shm/gapmem"); + int fd = open("/dev/shm/gapmem", O_RDWR | O_CREAT | O_EXCL, 0600); + if (fd < 0) { + fputs("Fatal error setting up multiheap\n", stderr); + SyExit(2); + } + + if (ftruncate(fd, size) < 0) { + fputs("Fatal error setting up multiheap!\n", stderr); + SyExit(2); + } + + for (int i = 0; i < membufcount; ++i) { + membufs[i] = + mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + if (membufs[i] == MAP_FAILED) { + fputs("Fatal error setting up multiheap!!\n", stderr); + SyExit(2); + } + } + + // Make the first membufs the earlier in memory. This is where the + // master points will live. + qsort(membufs, membufcount, sizeof(void *), order_pointers); + return membufs[0]; +} + +int SyTryToIncreasePool(void) +{ + return -1; +} + +#else #ifdef HAVE_MADVISE #ifndef MAP_ANONYMOUS @@ -802,6 +893,7 @@ void SyMAdviseFree(void) { #endif } + void *SyAnonMMap(size_t size) { void *result; size = SyRoundUpToPagesize(size); @@ -859,6 +951,7 @@ int SyTryToIncreasePool(void) return -1; /* Refuse */ } +#endif #endif int halvingsdone = 0; @@ -1684,38 +1777,51 @@ static Int preAllocAmount; /* These options must be kept in sync with those in system.g, so the help output is correct */ struct optInfo options[] = { - { 'B', "architecture", storeString, &SyArchitecture, 1}, /* default architecture needs to be passed from kernel - to library. Might be needed for autoload of compiled files */ - { 'C', "", processCompilerArgs, 0, 4}, /* must handle in kernel */ - { 'D', "debug-loading", toggle, &SyDebugLoading, 0}, /* must handle in kernel */ - { 'K', "maximal-workspace", storeMemory2, &SyStorKill, 1}, /* could handle from library with new interface */ - { 'L', "", storeString, &SyRestoring, 1}, /* must be handled in kernel */ - { 'M', "", toggle, &SyUseModule, 0}, /* must be handled in kernel */ - { 'R', "", unsetString, &SyRestoring, 0}, /* kernel */ - { 'a', "", storeMemory, &preAllocAmount, 1 }, /* kernel -- is this still useful */ - { 'e', "", toggle, &SyCTRD, 0 }, /* kernel */ - { 'f', "", forceLineEditing, (void *)2, 0 }, /* probably library now */ - { 'E', "", toggle, &SyUseReadline, 0 }, /* kernel */ - { 'l', "roots", setGapRootPath, 0, 1}, /* kernel */ - { 'm', "", storeMemory2, &SyStorMin, 1 }, /* kernel */ - { 'r', "", toggle, &IgnoreGapRC, 0 }, /* kernel */ - { 's', "", storeMemory, &SyAllocPool, 1 }, /* kernel */ - { 'n', "", forceLineEditing, 0, 0}, /* prob library */ - { 'o', "", storeMemory2, &SyStorMax, 1 }, /* library with new interface */ - { 'p', "", toggle, &SyWindow, 0 }, /* ?? */ - { 'q', "", toggle, &SyQuiet, 0 }, /* ?? */ + { 'B', "architecture", storeString, &SyArchitecture, + 1 }, /* default architecture needs to be passed from kernel + to library. Might be needed for autoload of compiled files */ + { 'C', "", processCompilerArgs, 0, 4 }, /* must handle in kernel */ + { 'D', "debug-loading", toggle, &SyDebugLoading, + 0 }, /* must handle in kernel */ + { 'K', "maximal-workspace", storeMemory2, &SyStorKill, + 1 }, /* could handle from library with new interface */ + { 'L', "", storeString, &SyRestoring, 1 }, /* must be handled in kernel */ + { 'M', "", toggle, &SyUseModule, 0 }, /* must be handled in kernel */ + { 'R', "", unsetString, &SyRestoring, 0 }, /* kernel */ + { 'a', "", storeMemory, &preAllocAmount, + 1 }, /* kernel -- is this still useful */ + { 'e', "", toggle, &SyCTRD, 0 }, /* kernel */ + { 'f', "", forceLineEditing, (void *)2, 0 }, /* probably library now */ + { 'E', "", toggle, &SyUseReadline, 0 }, /* kernel */ + { 'l', "roots", setGapRootPath, 0, 1 }, /* kernel */ + { 'm', "", storeMemory2, &SyStorMin, 1 }, /* kernel */ + { 'r', "", toggle, &IgnoreGapRC, 0 }, /* kernel */ + { 's', "", storeMemory, &SyAllocPool, 1 }, /* kernel */ + { 'n', "", forceLineEditing, 0, 0 }, /* prob library */ + { 'o', "", storeMemory2, &SyStorMax, 1 }, /* library with new interface */ + { 'p', "", toggle, &SyWindow, 0 }, /* ?? */ + { 'q', "", toggle, &SyQuiet, 0 }, /* ?? */ #ifdef HPCGAP - { 'S', "", toggle, &ThreadUI, 0 }, /* Thread UI */ - { 'Z', "", toggle, &DeadlockCheck, 0 }, /* Thread UI */ - { 'P', "", storePosInteger, &SyNumProcessors, 1 }, /* Thread UI */ - { 'G', "", storePosInteger, &SyNumGCThreads, 1 }, /* Thread UI */ + { 'S', "", toggle, &ThreadUI, 0 }, /* Thread UI */ + { 'Z', "", toggle, &DeadlockCheck, 0 }, /* Thread UI */ + { 'P', "", storePosInteger, &SyNumProcessors, 1 }, /* Thread UI */ + { 'G', "", storePosInteger, &SyNumGCThreads, 1 }, /* Thread UI */ +#endif + /* The following three options must be handled in the kernel so they + happen early enough */ + { 0, "prof", enableProfilingAtStartup, 0, + 1 }, /* enable profiling at startup */ + { 0, "memprof", enableMemoryProfilingAtStartup, 0, + 1 }, /* enable memory profiling at startup */ + { 0, "cover", enableCodeCoverageAtStartup, 0, + 1 }, /* enable code coverage at startup */ + { 0, "quitonbreak", toggle, &SyQuitOnBreak, + 0 }, /* Quit GAP if we enter the break loop */ +#ifdef GAP_MEM_CHECK + { 0, "enableMemCheck", enableMemCheck, 0, 0 }, #endif - /* The following three options must be handled in the kernel so they happen early enough */ - { 0 , "prof", enableProfilingAtStartup, 0, 1}, /* enable profiling at startup */ - { 0 , "memprof", enableMemoryProfilingAtStartup, 0, 1 }, /* enable memory profiling at startup */ - { 0 , "cover", enableCodeCoverageAtStartup, 0, 1}, /* enable code coverage at startup */ - { 0 , "quitonbreak", toggle, &SyQuitOnBreak, 0}, /* Quit GAP if we enter the break loop */ - { 0, "", 0, 0, 0}}; + { 0, "", 0, 0, 0 } +}; Char ** SyOriginalArgv;