diff --git a/configure.ac b/configure.ac index 2771b5f7ce..42c05f14b5 100644 --- a/configure.ac +++ b/configure.ac @@ -204,6 +204,22 @@ AS_CASE([$with_gc], ) AC_MSG_RESULT([$with_gc]) +dnl +dnl User setting: disable guards (race protection) +dnl + +AC_ARG_ENABLE([guards], + [AS_HELP_STRING([--enable-guards], + [enable guards for race protection in HPC-GAP])], + [enable_guards=$enableval], + [enable_guards=no]) + +AS_IF([[test "x$enable_guards" == "xyes"]], [ + AC_DEFINE([USE_HPC_GUARDS], [1], [define as 1 if guards should be checked]) +]) +AC_MSG_CHECKING([whether to use guards for race protection in HPC-GAP]) +AC_MSG_RESULT([$enable_guards]) + dnl dnl User setting: native thread-local storage (off by default) dnl See src/hpc/tls.h for more details on thread-local storage options. @@ -891,6 +907,9 @@ AS_IF([test "x$enable_hpcgap" = xyes],[ AS_BOX([WARNING: Experimental HPC-GAP mode enabled]) dnl also enable backtrace, to help debug spurious crashes AC_DEFINE([GAP_PRINT_BACKTRACE], [1], [to enable backtraces upon crashes]) + AS_IF([test "x$enable_guards" = xno], [ + AS_BOX([WARNING: HPC-GAP guards (race condition protection) are disabled.]) + ]) ]) AS_IF([test "x$enable_macos_tls_asm" = xno],[ diff --git a/etc/ci-prepare.sh b/etc/ci-prepare.sh index 6fc087ae88..e204d2b13c 100644 --- a/etc/ci-prepare.sh +++ b/etc/ci-prepare.sh @@ -23,7 +23,7 @@ BUILDDIR=$PWD if [[ $HPCGAP = yes ]] then - CONFIGFLAGS="--enable-hpcgap $CONFIGFLAGS" + CONFIGFLAGS="--enable-hpcgap --enable-guards $CONFIGFLAGS" fi @@ -40,8 +40,22 @@ then fi -# configure and make GAP +# configure time "$SRCDIR/configure" --enable-Werror $CONFIGFLAGS + +if [[ $HPCGAP = yes ]] +then + git clone https://github.com/rbehrends/unward + cd unward + ./configure CC=gcc CXX=g++ CFLAGS=-O2 CXXFLAGS=-O2 + make + cd .. + unward/bin/unward --inplace src + # commit the result to prevent the docomp test from triggering + git commit -m "Unward" src +fi + +# build GAP time make V=1 -j4 # download packages; instruct wget to retry several times if the diff --git a/src/gasman.h b/src/gasman.h index 66f8155141..acd911f635 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -38,6 +38,11 @@ #include "system.h" +#ifdef HPCGAP +#include "hpc/region.h" +#include "hpc/tls.h" +#endif + /**************************************************************************** ** @@ -293,18 +298,71 @@ EXPORT_INLINE UInt SIZE_BAG_CONTENTS(const void *ptr) { ** the application must inform {\Gasman} that it has changed the bag, by ** calling 'CHANGED_BAG(old)' in the above example (see "CHANGED_BAG"). */ +#ifdef HPCGAP +EXPORT_INLINE int ReadCheck(Bag bag) +{ + Region *region; + region = REGION(bag); + if (!region) + return 1; + if (region->owner == GetTLS()) + return 1; + if (region->readers[TLS(threadID)]) + return 1; + return 0; +} + +EXPORT_INLINE int WriteCheck(Bag bag) +{ + Region *region; + region = REGION(bag); + return !region || region->owner == GetTLS(); +} + +extern void HandleReadGuardError(Bag); +extern void HandleWriteGuardError(Bag); +#endif // HPCGAP + EXPORT_INLINE Bag *PTR_BAG(Bag bag) { GAP_ASSERT(bag != 0); +#ifdef USE_HPC_GUARDS + if (!WriteCheck(bag)) + HandleWriteGuardError(bag); +#endif return *(Bag**)bag; } +#ifdef USE_HPC_GUARDS +EXPORT_INLINE Bag *UNSAFE_PTR_BAG(Bag bag) +{ + GAP_ASSERT(bag != 0); + return *(Bag**)bag; +} +#else +#define UNSAFE_PTR_BAG PTR_BAG +#endif + EXPORT_INLINE const Bag *CONST_PTR_BAG(Bag bag) { GAP_ASSERT(bag != 0); +#ifdef USE_HPC_GUARDS + if (!ReadCheck(bag)) + HandleReadGuardError(bag); +#endif return *(const Bag * const *)bag; } +#ifdef USE_HPC_GUARDS +EXPORT_INLINE const Bag *UNSAFE_CONST_PTR_BAG(Bag bag) +{ + GAP_ASSERT(bag != 0); + return *(const Bag * const *)bag; +} +#else +#define UNSAFE_CONST_PTR_BAG CONST_PTR_BAG +#endif + EXPORT_INLINE void SET_PTR_BAG(Bag bag, Bag *val) { GAP_ASSERT(bag != 0); diff --git a/src/hpc/guards.h b/src/hpc/guards.h index 8e04e6a95a..86ea5adb2b 100644 --- a/src/hpc/guards.h +++ b/src/hpc/guards.h @@ -11,6 +11,7 @@ #ifndef GAP_HPC_GUARD_H #define GAP_HPC_GUARD_H +#include "gasman.h" #include "hpc/region.h" #include "hpc/tls.h" @@ -18,101 +19,64 @@ #error This header is only meant to be used with HPC-GAP #endif -#ifdef VERBOSE_GUARDS -void WriteGuardError(Bag bag, - const char *file, unsigned line, const char *func, const char *expr); -void ReadGuardError(Bag bag, - const char *file, unsigned line, const char *func, const char *expr); -#else -void WriteGuardError(Bag bag); -void ReadGuardError(Bag bag); -#endif - -#ifdef VERBOSE_GUARDS -#define WriteGuard(bag) WriteGuardFull(bag, __FILE__, __LINE__, __FUNCTION__, #bag) -static ALWAYS_INLINE Bag WriteGuardFull(Bag bag, - const char *file, unsigned line, const char *func, const char *expr) -#else static ALWAYS_INLINE Bag WriteGuard(Bag bag) -#endif { - Region *region; - if (!IS_BAG_REF(bag)) + if (!WriteCheck(bag)) + HandleWriteGuardError(bag); return bag; - region = REGION(bag); - if (region && region->owner != GetTLS() && region->alt_owner != GetTLS()) - WriteGuardError(bag -#ifdef VERBOSE_GUARDS - , file, line, func, expr -#endif - ); - return bag; } + EXPORT_INLINE Bag ImpliedWriteGuard(Bag bag) { - return bag; + return bag; } EXPORT_INLINE int CheckWriteAccess(Bag bag) { - Region *region; - if (!IS_BAG_REF(bag)) - return 1; - region = REGION(bag); - return !(region && region->owner != GetTLS() && region->alt_owner != GetTLS()) - || TLS(DisableGuards) >= 2; + Region * region; + if (!IS_BAG_REF(bag)) + return 1; + region = REGION(bag); + return !(region && region->owner != GetTLS() && + region->alt_owner != GetTLS()) || + TLS(DisableGuards) >= 2; } EXPORT_INLINE int CheckExclusiveWriteAccess(Bag bag) { - Region *region; - if (!IS_BAG_REF(bag)) - return 1; - region = REGION(bag); - if (!region) - return 0; - return region->owner == GetTLS() || region->alt_owner == GetTLS() - || TLS(DisableGuards) >= 2; + Region * region; + if (!IS_BAG_REF(bag)) + return 1; + region = REGION(bag); + if (!region) + return 0; + return region->owner == GetTLS() || region->alt_owner == GetTLS() || + TLS(DisableGuards) >= 2; } -#ifdef VERBOSE_GUARDS -#define ReadGuard(bag) ReadGuardFull(bag, __FILE__, __LINE__, __FUNCTION__, #bag) -static ALWAYS_INLINE Bag ReadGuardFull(Bag bag, - const char *file, unsigned line, const char *func, const char *expr) -#else static ALWAYS_INLINE Bag ReadGuard(Bag bag) -#endif { - Region *region; - if (!IS_BAG_REF(bag)) + if (!ReadCheck(bag)) + HandleReadGuardError(bag); return bag; - region = REGION(bag); - if (region && region->owner != GetTLS() && - !region->readers[TLS(threadID)] && region->alt_owner != GetTLS()) - ReadGuardError(bag -#ifdef VERBOSE_GUARDS - , file, line, func, expr -#endif - ); - return bag; } - static ALWAYS_INLINE Bag ImpliedReadGuard(Bag bag) { - return bag; + return bag; } static ALWAYS_INLINE int CheckReadAccess(Bag bag) { - Region *region; - if (!IS_BAG_REF(bag)) - return 1; - region = REGION(bag); - return !(region && region->owner != GetTLS() && - !region->readers[TLS(threadID)] && region->alt_owner != GetTLS()) - || TLS(DisableGuards) >= 2; + Region * region; + if (!IS_BAG_REF(bag)) + return 1; + region = REGION(bag); + return !(region && region->owner != GetTLS() && + !region->readers[TLS(threadID)] && + region->alt_owner != GetTLS()) || + TLS(DisableGuards) >= 2; } -#endif // GAP_HPC_GUARD_H +#endif // GAP_HPC_GUARD_H diff --git a/src/hpc/thread.c b/src/hpc/thread.c index 520e9b3726..53f1579e43 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -1309,73 +1309,82 @@ Region * CurrentRegion(void) return TLS(currentRegion); } -#ifdef VERBOSE_GUARDS - -static void PrintGuardError(char * buffer, - char * mode, - Obj obj, - const char * file, - unsigned line, - const char * func, - const char * expr) -{ - sprintf(buffer, "No %s access to object %llu of type %s\n" - "in %s, line %u, function %s(), accessing %s", - mode, (unsigned long long)(UInt)obj, TNAM_OBJ(obj), file, line, - func, expr); -} -void WriteGuardError(Obj o, - const char * file, - unsigned line, - const char * func, - const char * expr) -{ - char * buffer = alloca(strlen(file) + strlen(func) + strlen(expr) + 200); - ImpliedReadGuard(o); - if (TLS(DisableGuards)) - return; - SetGVar(&LastInaccessibleGVar, o); - PrintGuardError(buffer, "write", o, file, line, func, expr); - ErrorMayQuit("%s", (UInt)buffer, 0L); -} +#ifdef DEBUG_GUARDS +extern GVarDescriptor GUARD_ERROR_STACK; +#endif + +// These are temporary debugging functions. + +static Int NumReadErrors = 0; +static Int NumWriteErrors = 0; + +#ifdef DEBUG_GUARDS -void ReadGuardError(Obj o, - const char * file, - unsigned line, - const char * func, - const char * expr) +#include + +#define BACKTRACE_DEPTH 128 + +// Record the backtrace in a global GAP variable. + +void SetGuardErrorStack(void) { - char * buffer = alloca(strlen(file) + strlen(func) + strlen(expr) + 200); - ImpliedReadGuard(o); - if (TLS(DisableGuards)) - return; - SetGVar(&LastInaccessibleGVar, o); - PrintGuardError(buffer, "read", o, file, line, func, expr); - ErrorMayQuit("%s", (UInt)buffer, 0L); + void * callstack[BACKTRACE_DEPTH]; + int depth = backtrace(callstack, BACKTRACE_DEPTH); + char ** calls = backtrace_symbols(callstack, depth); + Obj stack = NEW_PLIST_IMM(T_PLIST, depth); + SET_LEN_PLIST(stack, depth - 1); + for (int i = 1; i < depth; i++) { + char * p = calls[i] + 1; + char * q = p; + while (*p) { + if (p[-1] == ' ' && p[0] == ' ') + p++; + else + *q++ = *p++; + } + *q++ = 0; + SET_ELM_PLIST(stack, i, MakeImmString(calls[i])); + CHANGED_BAG(stack); + } + free(calls); + SetGVar(&GUARD_ERROR_STACK, stack); } +#endif -#else -void WriteGuardError(Obj o) +void HandleReadGuardError(Bag bag) { - ImpliedReadGuard(o); + NumReadErrors++; + // We shift some of the rarer checks here to + // avoid overloading ReadCheck(). + if (REGION(bag)->alt_owner == GetTLS()) + return; if (TLS(DisableGuards)) return; - SetGVar(&LastInaccessibleGVar, o); + SetGVar(&LastInaccessibleGVar, bag); +#ifdef DEBUG_GUARDS + SetGuardErrorStack(); +#endif ErrorMayQuit( - "Attempt to write object %i of type %s without having write access", - (Int)o, (Int)TNAM_OBJ(o)); + "Attempt to read object %i of type %s without having read access", + (Int)bag, (Int)TNAM_OBJ(bag)); } -void ReadGuardError(Obj o) +void HandleWriteGuardError(Bag bag) { - ImpliedReadGuard(o); + NumWriteErrors++; + // We shift some of the rarer checks here to + // avoid overloading ReadCheck(). + if (REGION(bag)->alt_owner == GetTLS()) + return; if (TLS(DisableGuards)) return; - SetGVar(&LastInaccessibleGVar, o); + SetGVar(&LastInaccessibleGVar, bag); +#ifdef DEBUG_GUARDS + SetGuardErrorStack(); +#endif ErrorMayQuit( - "Attempt to read object %i of type %s without having read access", - (Int)o, (Int)TNAM_OBJ(o)); + "Attempt to write object %i of type %s without having write access", + (Int)bag, (Int)TNAM_OBJ(bag)); } -#endif #endif diff --git a/src/hpc/threadapi.c b/src/hpc/threadapi.c index ca6cd0274f..4e9c27bd0e 100644 --- a/src/hpc/threadapi.c +++ b/src/hpc/threadapi.c @@ -959,6 +959,9 @@ static void PrintRegion(Obj); GVarDescriptor LastInaccessibleGVar; static GVarDescriptor MAX_INTERRUPTGVar; +#ifdef DEBUG_GUARDS +GVarDescriptor GUARD_ERROR_STACK; +#endif static UInt RNAM_SIGINT; static UInt RNAM_SIGCHLD; @@ -2663,6 +2666,9 @@ static Int InitKernel(StructInitInfo * module) DeclareGVar(&LastInaccessibleGVar, "LastInaccessible"); DeclareGVar(&MAX_INTERRUPTGVar, "MAX_INTERRUPT"); +#ifdef DEBUG_GUARDS + DeclareGVar(&GUARD_ERROR_STACK, "GUARD_ERROR_STACK"); +#endif // install mark functions InitMarkFuncBags(T_THREAD, MarkNoSubBags); diff --git a/src/system.h b/src/system.h index 79d6c7bdcd..27e6a0ee36 100644 --- a/src/system.h +++ b/src/system.h @@ -63,6 +63,12 @@ GAP_STATIC_ASSERT(sizeof(void *) == SIZEOF_VOID_P, "sizeof(void *) is wrong"); #endif #endif +// If we are not running HPC-GAP, disable read and write guards. + +#ifndef HPCGAP +#undef USE_HPC_GUARDS +#endif + /**************************************************************************** ** *S GAP_PATH_MAX . . . . . . . . . . . . size for buffers storing file paths