From f1ede8c33a237c2b770bd0b95335e4cd85fded2a Mon Sep 17 00:00:00 2001 From: "Erik M. Bray" Date: Thu, 6 Dec 2018 16:11:35 +0000 Subject: [PATCH] GAP_Enter/Leave macros to bracket use of libgap and stack local GAP objects in code which embeds libgap There are two parts to this: First, the outer-most GAP_Enter() must set the StackBottom variable for GASMAN, without which objects tracked by GASMAN that are allocated on the stack are properly tracked (see #3089). Second, the outer-most GAP_Enter() call should set a jump point for longjmps to STATE(ReadJmpError). Code within the GAP kernel may reset this, but we should set it here in case any unexpected errors occur within the GAP kernel that are not already handled appropriately by a TRY_IF_NO_ERROR. Or, more precisely, since TRY_IF_NO_ERROR does not currently *clear* the jump buffer it sets even when leaving the block without any errors found, a subsequent error that is not within a TRY_IF_NO_ERROR block can still result in a jump to the last TRY_IF_NO_ERROR block which was entered, which can be quite arbitrary (e.g. inside IntializeGap). For the first issue, we add GAP_EnterStack() and GAP_LeaveStack() macros which implement *just* the StackBottom handling without any other error handling. We also add a function to gasman.c called SetStackBottomBags which just updates the StackBottom variable. Then the GAP_EnterStack() macro can be used within a function to set StackBottom to somewhere at or near the beginning of that function's stack frame. This uses GCC's __builtin_frame_address, but supported is probably needed for other platforms that don't have this. The global variable EnterStackCount in libgap-api.c is used to track recursive calls into GAP_EnterStack(). We only want to set StackBottom on the outer-most call. The count is decremented on GAP_LeaveStack(). For setting the STATE(ReadJmpError) jump buffer we provide a macro called GAP_Error_Setjmp() which is fairly straightforward, except that it needs to be written in such a way that it can be used in concert correctly with GAP_EnterStack(). In particular, if returning to the site of a GAP_Error_Setjmp() call we do not want to accidentally re-increment the EnterStackCount. Finally, the higher-level GAP_Enter() and GAP_Leave() macros are provided: The latter is just an alias for GAP_LeaveStack(), but the former carefully combines *both* GAP_Error_Setjmp() and GAP_EnterStack(). Although called like a function, the GAP_Enter() macro expands to a compound statement (necessary for both GAP_EnterStack() and GAP_Error_Setjmp() to work properly). The order of expansion is also deliberate so that this can be used like: jmp_retval = GAP_Enter(); so that the return value of the setjmp call can be stored in a variable while using GAP_Enter(), and can be checked to see whether an error occurred. However, this requires some care to ensure that the following GAP_EnterStack() doesn't increment the EnterStackCount following a return to this point via a longjmp. Also updates the libgap API tests to demonstrate usage of GAP_Enter/Leave(). Note: The whole EnterStackCount manipulation is really really only in service to GASMAN and can be disabled entirely when compiled for a different GC, but it still has some use as a sanity check on GAP_Enter/Leave() so for not it is mostly but not entirely a no-op on GAP built with other GCs. --- src/gasman.c | 7 +++ src/gasman.h | 2 + src/libgap-api.c | 65 ++++++++++++++++++++++++ src/libgap-api.h | 128 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 202 insertions(+) diff --git a/src/gasman.c b/src/gasman.c index 13c0b1ed59..417811401e 100644 --- a/src/gasman.c +++ b/src/gasman.c @@ -1193,6 +1193,13 @@ void SetExtraMarkFuncBags(TNumExtraMarkFuncBags func) } + +void SetStackBottomBags(void * StackBottom) +{ + StackBottomBags = StackBottom; +} + + void InitBags ( UInt initial_size, Bag * stack_bottom, diff --git a/src/gasman.h b/src/gasman.h index 236eb8bbf9..ab3b3bf96c 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -973,6 +973,8 @@ typedef void (* TNumCollectFuncBags) ( void ); extern void InitCollectFuncBags ( TNumCollectFuncBags before_func, TNumCollectFuncBags after_func ); + +extern void SetStackBottomBags(void * StackBottom); #endif // ExtraMarkFuncBags, if not NULL, is called during garbage collection diff --git a/src/libgap-api.c b/src/libgap-api.c index 73acbb3342..be3713ebc2 100644 --- a/src/libgap-api.c +++ b/src/libgap-api.c @@ -1,5 +1,7 @@ // LibGAP API - API for using GAP as shared library. +#include + #include "libgap-api.h" #include "ariths.h" @@ -7,6 +9,7 @@ #include "calls.h" #include "gap.h" #include "gapstate.h" +#include "gasman.h" #include "gvars.h" #include "integer.h" #include "lists.h" @@ -354,3 +357,65 @@ Obj GAP_CharWithValue(UChar obj) { return ObjsChar[obj]; } + +syJmp_buf * GAP_GetReadJmpError(void) +{ + return &(STATE(ReadJmpError)); +} + + +static volatile sig_atomic_t EnterStackCount = 0; + + +// These are wrapped by the macros GAP_EnterStack() and GAP_LeaveStack() +// respectively. +void GAP_EnterStack_(void * StackTop) +{ + if (EnterStackCount < 0) { + EnterStackCount = -EnterStackCount; + } + else { + if (EnterStackCount == 0) { +#ifdef USE_GASMAN + SetStackBottomBags(StackTop); +#endif + } + EnterStackCount++; + } +} + +void GAP_LeaveStack_(void) +{ + EnterStackCount--; +} + +void GAP_EnterDebugMessage(char * message, char * file, int line) +{ + fprintf(stderr, "%s: %d; %s:%d\n", message, EnterStackCount, file, + line); +} + +int GAP_Error_Prejmp_(const char * file, int line) +{ + GAP_ENTER_DEBUG_MESSAGE("Error_Prejmp", file, line); + if (EnterStackCount > 0) { + return 1; + } + return 0; +} + +/* Helper function for GAP_Error_Postjmp_ (see libgap-api.h) which manipulates + * EnterStackCount in the (generally unlikely) case of returning from a longjmp + */ +void GAP_Error_Postjmp_Returning_(void) +{ + /* This only should have been called from the outer-most + * GAP_EnterStack() call so make sure it resets the EnterStackCount; + * We set EnterStackCount to its negative which indicates to + * GAP_EnterStack that we just returned from a long jump and should + * reset EnterStackCount to its value at the return point rather than + * increment it again */ + if (EnterStackCount > 0) { + EnterStackCount = -EnterStackCount; + } +} diff --git a/src/libgap-api.h b/src/libgap-api.h index 231092087e..2d18ded274 100644 --- a/src/libgap-api.h +++ b/src/libgap-api.h @@ -6,6 +6,134 @@ #include "system.h" +#ifdef __GNUC__ +#define GAP_unlikely(x) __builtin_expect(!!(x), 0) +#else +#define GAP_unlikely(x) (x) +#endif + + +#ifndef GAP_ENTER_DEBUG +#define GAP_ENTER_DEBUG 0 +#endif + + +extern syJmp_buf * GAP_GetReadJmpError(void); +extern void GAP_EnterDebugMessage_(char * message, char * file, int line); +extern void GAP_EnterStack_(void *); +extern void GAP_LeaveStack_(void); +extern int GAP_Error_Prejmp_(const char *, int); +extern void GAP_Error_Postjmp_Returning_(void); + + +static inline int GAP_Error_Postjmp_(int JumpRet) +{ + if (GAP_unlikely(JumpRet != 0)) { + GAP_Error_Postjmp_Returning_(); + return 0; + } + + return 1; +} + + +#if GAP_ENTER_DEBUG +#define GAP_ENTER_DEBUG_MESSAGE(message, file, line) \ + GAP_EnterDebugMessage_(message, file, line) +#else +#define GAP_ENTER_DEBUG_MESSAGE(message, file, line) \ + do { \ + } while (0) +#endif + + +// Code which uses the GAP API and/or which keeps references to any GAP +// objects in local variables must be bracketed by uses of GAP_EnterStack() +// and GAP_LeaveStack(), in particular when using the GASMAN garbage +// collector; otherwise GAP objects may be garbage collected while still in +// use. +// +// In general user code should use the more general GAP_Enter()/Leave() macros +// defined below, as these also specify a terminal point for unhandled GAP +// errors to bubble up to. However, GAP_EnterStack() and GAP_LeaveStack() +// should still be used in the defintion of a custom error handling callback as +// passed to GAP_Initialize(). Using the more general GAP_Enter() in this case +// will result in crashes if the error handler is entered recursively (you +// don't want the GAP error handling code to cause a longjmp into the error +// callback itself since then the error callback will never be returned from). +#ifdef __GNUC__ +#define GAP_EnterStack() \ + do { \ + GAP_ENTER_DEBUG_MESSAGE("EnterStack", __FILE__, __LINE__); \ + GAP_EnterStack_(__builtin_frame_address(0)); \ + } while (0) +#elif defined(USE_GASMAN) +#error GASMAN requires a way to get the current stack frame base address \ + for the GAP_EnterStack() macro; normally this uses the \ + __builtin_frame_address GNU extension so if this is not available \ + it is necessary to provide your own implementation here. +#else +// If we're not using GASMAN in the first place GAP_EnterStack_() is not +// strictly needed, and can just be called with a dummy value +#define GAP_EnterStack() \ + do { \ + GAP_ENTER_DEBUG_MESSAGE("EnterStack", __FILE__, __LINE__); \ + GAP_EnterStack_(0); \ + } while (0) +#endif + +#define GAP_LeaveStack() \ + GAP_LeaveStack_(); + + +#define GAP_Error_Setjmp() \ + (GAP_unlikely(GAP_Error_Prejmp_(__FILE__, __LINE__)) || \ + GAP_Error_Postjmp_(sySetjmp(*GAP_GetReadJmpError()))) + + +// Code which uses the GAP API exposed by this header file should sandwich any +// such calls between uses of the GAP_Enter() and GAP_Leave() macro as follows: +// +// int ok = GAP_Enter(); +// if (ok) { +// ... // any number of calls to GAP APIs +// } +// GAP_Leave(); +// +// This is in particular crucial if your code keeps references to any GAP +// functions in local variables: Calling GAP_Enter() ensures that GAP is aware +// of such references, and will not garbage collect the referenced objects. +// Failing to use these macros properly can lead to crashes, or worse, silent +// memory corruption. You have been warned! +// +// Note that due to the implementation of these macros, you unfortunately +// cannot "simplify" the above example code to: +// +// if (GAP_Enter()) { ... } GAP_Leave(); +// +// Some notes on the implementation: +// +// GAP_Enter() is a combination of GAP_Error_Setjmp() and GAP_EnterStack(). +// It must call GAP_Error_Setjmp() first, to ensure that writing +// ``int ok = GAP_Enter();'' works as intended (the value assigned to ok then +// is the return value of GAP_Error_Setjmp). +// +// * GAP_EnterStack() defined and explained above must be a macro since it +// needs to figure out (to the extent possible) the base address of the stack +// frame from which it is called. +// +// * GAP_Error_Setjmp() effectively calls setjmp to the STATE(ReadJmpError) +// longjmp buffer, so that read errors which occur in GAP that are not +// otherwise "handled" by a TRY_IF_NO_ERROR { } block have a logical place +// to return to. It returns 1 if no error occurred, and 0 if returning from +// an error. +#define GAP_Enter() \ + GAP_Error_Setjmp(); \ + GAP_EnterStack() + +#define GAP_Leave() GAP_LeaveStack() + + //// //// Setup and initialisation ////