From 115687e7224650b8e28146a6e086c7b84431d484 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Fri, 15 Nov 2019 15:22:26 +0100 Subject: [PATCH 01/14] Make HPC-GAP guards work without Ward. --- configure.ac | 19 ++++++++ etc/ci-prepare.sh | 18 ++++++- src/gasman.h | 58 ++++++++++++++++++++++ src/hpc/guards.h | 100 ++++++++++++-------------------------- src/hpc/thread.c | 115 ++++++++++++++++++++++++-------------------- src/hpc/threadapi.c | 6 +++ src/system.h | 6 +++ 7 files changed, 199 insertions(+), 123 deletions(-) diff --git a/configure.ac b/configure.ac index faff01e44c..85deb1a5d2 100644 --- a/configure.ac +++ b/configure.ac @@ -248,6 +248,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. @@ -978,6 +994,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 9383d16f4a..51d24a94b2 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 # Use alternative downloader which retries on failure and uses the Travis cache diff --git a/src/gasman.h b/src/gasman.h index 208ddab849..8dc8f3d465 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -38,6 +38,11 @@ #include "common.h" +#ifdef HPCGAP +#include "hpc/region.h" +#include "hpc/tls.h" +#endif + /**************************************************************************** ** @@ -305,18 +310,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 7ce7822d6a..48dd4a1b1a 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -1311,73 +1311,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, 0); -} +#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, 0); + 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 aa0812737c..a44214caab 100644 --- a/src/hpc/threadapi.c +++ b/src/hpc/threadapi.c @@ -929,6 +929,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; @@ -2617,6 +2620,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 95b2eba4a3..e921b23d91 100644 --- a/src/system.h +++ b/src/system.h @@ -25,6 +25,12 @@ #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 From ed182a561669b8fcf75cf42b646fe2a9eedbb7e8 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Mon, 2 Dec 2019 10:49:52 +0100 Subject: [PATCH 02/14] Clean up HPC-GAP changes. --- src/hpc/thread.c | 13 +++++++++++-- src/system.h | 6 ++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/hpc/thread.c b/src/hpc/thread.c index 48dd4a1b1a..f1279a9d0d 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -1311,12 +1311,21 @@ Region * CurrentRegion(void) return TLS(currentRegion); } +// Kernel debugging information for guards. +// +// When compiled with -DDEBUG_GUARDS, the GAP variable GUARD_ERROR_STACK +// will contain the C stack after an error, allowing us to located where +// it occurred without having to fire up a debugger first. +// +// The variables NumReadErrors and NumWriteErrors track the number of +// failed guard checks. These are used in conjunction with the low-level +// function DISABLE_GUARDS() in order to track how many guard failures +// occur in a given section of code. + #ifdef DEBUG_GUARDS extern GVarDescriptor GUARD_ERROR_STACK; #endif -// These are temporary debugging functions. - static Int NumReadErrors = 0; static Int NumWriteErrors = 0; diff --git a/src/system.h b/src/system.h index e921b23d91..4ff10419bd 100644 --- a/src/system.h +++ b/src/system.h @@ -25,10 +25,12 @@ #endif -// If we are not running HPC-GAP, disable read and write guards. +// If we are not running HPC-GAP, guards should be disabled #ifndef HPCGAP -#undef USE_HPC_GUARDS +#ifdef USE_HPC_GUARDS +#error Do not use --enable-guards without --enable-hpcgap. +#endif #endif /**************************************************************************** From 61f7972aab87283df1e8c256e62dce3d77936dcc Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sat, 1 Feb 2020 17:33:14 +0100 Subject: [PATCH 03/14] Remove implied guards. These existed to assist Ward's data flow analysis and serve no purpose without Ward. --- src/gvars.c | 2 -- src/hpc/guards.h | 10 ---------- src/hpc/thread.c | 7 ++++--- src/lists.c | 1 - 4 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/gvars.c b/src/gvars.c index 1053e73e61..cc1511a7e8 100644 --- a/src/gvars.c +++ b/src/gvars.c @@ -1516,7 +1516,6 @@ Obj GVarFunction(GVarDescriptor *gvar) ErrorQuit("Global variable '%s' not initialized", (UInt)(gvar->name), 0); if (REGION(result)) ErrorQuit("Global variable '%s' is not a function", (UInt)(gvar->name), 0); - ImpliedWriteGuard(result); if (TNUM_OBJ(result) != T_FUNCTION) ErrorQuit("Global variable '%s' is not a function", (UInt)(gvar->name), 0); MEMBAR_READ(); @@ -1530,7 +1529,6 @@ Obj GVarOptFunction(GVarDescriptor *gvar) return (Obj) 0; if (REGION(result)) return (Obj) 0; - ImpliedWriteGuard(result); if (TNUM_OBJ(result) != T_FUNCTION) return (Obj) 0; MEMBAR_READ(); diff --git a/src/hpc/guards.h b/src/hpc/guards.h index 86ea5adb2b..c77e8f5f82 100644 --- a/src/hpc/guards.h +++ b/src/hpc/guards.h @@ -27,11 +27,6 @@ static ALWAYS_INLINE Bag WriteGuard(Bag bag) } -EXPORT_INLINE Bag ImpliedWriteGuard(Bag bag) -{ - return bag; -} - EXPORT_INLINE int CheckWriteAccess(Bag bag) { Region * region; @@ -62,11 +57,6 @@ static ALWAYS_INLINE Bag ReadGuard(Bag bag) return bag; } -static ALWAYS_INLINE Bag ImpliedReadGuard(Bag bag) -{ - return bag; -} - static ALWAYS_INLINE int CheckReadAccess(Bag bag) { Region * region; diff --git a/src/hpc/thread.c b/src/hpc/thread.c index f1279a9d0d..2aecbe2e45 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -1313,9 +1313,10 @@ Region * CurrentRegion(void) // Kernel debugging information for guards. // -// When compiled with -DDEBUG_GUARDS, the GAP variable GUARD_ERROR_STACK -// will contain the C stack after an error, allowing us to located where -// it occurred without having to fire up a debugger first. +// When DEBUG_GUARDS is #defined, the GAP variable GUARD_ERROR_STACK +// is set to a GAP plain list describing the C stack after an error, +// allowing us to located where it occurred without having to fire up a +// debugger first. // // The variables NumReadErrors and NumWriteErrors track the number of // failed guard checks. These are used in conjunction with the low-level diff --git a/src/lists.c b/src/lists.c index d41d7e684e..74afb067f8 100644 --- a/src/lists.c +++ b/src/lists.c @@ -145,7 +145,6 @@ static Obj AttrLENGTH(Obj self, Obj list) /* internal list types */ #ifdef HPCGAP ReadGuard(list); - ImpliedWriteGuard(list); if ( (FIRST_LIST_TNUM<=TNUM_OBJ(list) && TNUM_OBJ(list)<=LAST_LIST_TNUM) || TNUM_OBJ(list) == T_ALIST || TNUM_OBJ(list) == T_FIXALIST) { return ObjInt_Int( LEN_LIST(list) ); From b550ef9cf566f53a0d630c204e4e427964482b73 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sat, 1 Feb 2020 18:10:50 +0100 Subject: [PATCH 04/14] Check for --enable-guards/hpcgap consistency in configure.ac --- configure.ac | 7 +++++++ src/system.h | 8 -------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/configure.ac b/configure.ac index 85deb1a5d2..c6069bc618 100644 --- a/configure.ac +++ b/configure.ac @@ -264,6 +264,13 @@ AS_IF([[test "x$enable_guards" == "xyes"]], [ AC_MSG_CHECKING([whether to use guards for race protection in HPC-GAP]) AC_MSG_RESULT([$enable_guards]) +dnl +dnl Check: --enable-guards requires --enable-hpcgap +dnl + +AS_IF([[test "x$enable_guards" == "xyes" && test "x$enable_hpcgap" == "xno"]], + [AC_MSG_ERROR([Option --enable-guards requires --enable-hpcgap])]) + 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. diff --git a/src/system.h b/src/system.h index 4ff10419bd..95b2eba4a3 100644 --- a/src/system.h +++ b/src/system.h @@ -25,14 +25,6 @@ #endif -// If we are not running HPC-GAP, guards should be disabled - -#ifndef HPCGAP -#ifdef USE_HPC_GUARDS -#error Do not use --enable-guards without --enable-hpcgap. -#endif -#endif - /**************************************************************************** ** *S GAP_PATH_MAX . . . . . . . . . . . . size for buffers storing file paths From b6b175f1935ad0b298620774aab02241384d671b Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Mon, 3 Feb 2020 18:26:20 +0100 Subject: [PATCH 05/14] Fix guards for SET_TYPE_POSOBJ()/SET_TYPE_COMOBJ(). --- src/objects.c | 8 -------- src/objects.h | 12 ++++++++++++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/objects.c b/src/objects.c index 8989d72ca0..e09bdd0b36 100644 --- a/src/objects.c +++ b/src/objects.c @@ -208,18 +208,10 @@ void SET_TYPE_OBJ(Obj obj, Obj type) CHANGED_BAG(obj); break; case T_COMOBJ: -#ifdef HPCGAP - ReadGuard(obj); - MEMBAR_WRITE(); -#endif SET_TYPE_COMOBJ(obj, type); CHANGED_BAG(obj); break; case T_POSOBJ: -#ifdef HPCGAP - ReadGuard(obj); - MEMBAR_WRITE(); -#endif SET_TYPE_POSOBJ(obj, type); CHANGED_BAG(obj); break; diff --git a/src/objects.h b/src/objects.h index 53b2ef2e54..1a1732edf3 100644 --- a/src/objects.h +++ b/src/objects.h @@ -841,7 +841,13 @@ EXPORT_INLINE Obj TYPE_COMOBJ(Obj obj) */ EXPORT_INLINE void SET_TYPE_COMOBJ(Obj obj, Obj val) { +#ifdef HPCGAP + MEMBAR_WRITE(); + // Only require a read guard + ((Obj *) CONST_ADDR_OBJ(obj))[0] = val; +#else ADDR_OBJ(obj)[0] = val; +#endif } @@ -884,7 +890,13 @@ EXPORT_INLINE Obj TYPE_POSOBJ(Obj obj) */ EXPORT_INLINE void SET_TYPE_POSOBJ(Obj obj, Obj val) { +#ifdef HPCGAP + MEMBAR_WRITE(); + // Only require a read guard + ((Obj *) CONST_ADDR_OBJ(obj))[0] = val; +#else ADDR_OBJ(obj)[0] = val; +#endif } From 2a63fb8b269b2b0672a598193f77cc3b77297c29 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Wed, 5 Feb 2020 10:03:28 +0100 Subject: [PATCH 06/14] Remove unnecessary ReadGuard(). --- src/lists.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lists.c b/src/lists.c index 74afb067f8..a7880b67ed 100644 --- a/src/lists.c +++ b/src/lists.c @@ -144,7 +144,6 @@ static Obj AttrLENGTH(Obj self, Obj list) { /* internal list types */ #ifdef HPCGAP - ReadGuard(list); if ( (FIRST_LIST_TNUM<=TNUM_OBJ(list) && TNUM_OBJ(list)<=LAST_LIST_TNUM) || TNUM_OBJ(list) == T_ALIST || TNUM_OBJ(list) == T_FIXALIST) { return ObjInt_Int( LEN_LIST(list) ); From b75b53638b7c12ff41513b514021ab09f0bdea87 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Wed, 5 Feb 2020 11:15:03 +0100 Subject: [PATCH 07/14] Reduce code overhead from guard checks. --- src/gasman.h | 12 +++++++----- src/hpc/thread.c | 17 +++++++++-------- src/hpc/tls.h | 12 ------------ src/system.h | 12 ++++++++++++ 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/gasman.h b/src/gasman.h index 8dc8f3d465..f80ea5f57d 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -311,6 +311,8 @@ EXPORT_INLINE UInt SIZE_BAG_CONTENTS(const void *ptr) ** calling 'CHANGED_BAG(old)' in the above example (see "CHANGED_BAG"). */ #ifdef HPCGAP +extern int ExtendedGuardCheck(Bag) PURE_FUNC; + EXPORT_INLINE int ReadCheck(Bag bag) { Region *region; @@ -321,18 +323,18 @@ EXPORT_INLINE int ReadCheck(Bag bag) return 1; if (region->readers[TLS(threadID)]) return 1; - return 0; + return ExtendedGuardCheck(bag); } EXPORT_INLINE int WriteCheck(Bag bag) { - Region *region; + Region * region; region = REGION(bag); - return !region || region->owner == GetTLS(); + return !region || region->owner == GetTLS() || ExtendedGuardCheck(bag); } -extern void HandleReadGuardError(Bag); -extern void HandleWriteGuardError(Bag); +extern void HandleReadGuardError(Bag) NORETURN; +extern void HandleWriteGuardError(Bag) NORETURN; #endif // HPCGAP EXPORT_INLINE Bag *PTR_BAG(Bag bag) diff --git a/src/hpc/thread.c b/src/hpc/thread.c index 2aecbe2e45..3ca209da35 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -1363,15 +1363,20 @@ void SetGuardErrorStack(void) } #endif -void HandleReadGuardError(Bag bag) +int ExtendedGuardCheck(Bag bag) { - NumReadErrors++; // We shift some of the rarer checks here to // avoid overloading ReadCheck(). if (REGION(bag)->alt_owner == GetTLS()) - return; + return 1; if (TLS(DisableGuards)) - return; + return 1; + return 0; +} + +void HandleReadGuardError(Bag bag) +{ + NumReadErrors++; SetGVar(&LastInaccessibleGVar, bag); #ifdef DEBUG_GUARDS SetGuardErrorStack(); @@ -1386,10 +1391,6 @@ void HandleWriteGuardError(Bag bag) 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, bag); #ifdef DEBUG_GUARDS SetGuardErrorStack(); diff --git a/src/hpc/tls.h b/src/hpc/tls.h index eee4123729..c90f08042e 100644 --- a/src/hpc/tls.h +++ b/src/hpc/tls.h @@ -108,18 +108,6 @@ extern __thread ThreadLocalStorage *TLSInstance; #include -#ifdef HAVE_FUNC_ATTRIBUTE_PURE -#define PURE_FUNC __attribute__((pure)) -#else -#define PURE_FUNC -#endif - -#ifdef HAVE_FUNC_ATTRIBUTE_CONSTRUCTOR -#define CONSTRUCTOR_FUNC __attribute__((constructor)) -#else -#define CONSTRUCTOR_FUNC -#endif - #ifdef ALLOW_PURE_PTHREAD_GETSPECIFIC // pthread_getspecific() is not defined as pure by default; redeclaring // it as such works for gcc/clang and allows the compiler to hoist calls diff --git a/src/system.h b/src/system.h index 95b2eba4a3..f59a856f8f 100644 --- a/src/system.h +++ b/src/system.h @@ -106,6 +106,18 @@ enum { #define NOINLINE #endif +#ifdef HAVE_FUNC_ATTRIBUTE_PURE +#define PURE_FUNC __attribute__((pure)) +#else +#define PURE_FUNC +#endif + +#ifdef HAVE_FUNC_ATTRIBUTE_CONSTRUCTOR +#define CONSTRUCTOR_FUNC __attribute__((constructor)) +#else +#define CONSTRUCTOR_FUNC +#endif + /**************************************************************************** ** From b29c2d48e5ac89cd7039d4eb9cfaa4368ec54c82 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Fri, 7 Feb 2020 11:49:52 +0100 Subject: [PATCH 08/14] Fix concurrent T_DATOBJ/T_COMOBJ/T_POSOBJ type access. This also adds some missing memory barriers. --- src/objects.h | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/objects.h b/src/objects.h index 1a1732edf3..b712564def 100644 --- a/src/objects.h +++ b/src/objects.h @@ -831,7 +831,11 @@ EXPORT_INLINE BOOL IS_COMOBJ(Obj obj) */ EXPORT_INLINE Obj TYPE_COMOBJ(Obj obj) { - return CONST_ADDR_OBJ(obj)[0]; + Obj result = CONST_ADDR_OBJ(obj)[0]; +#ifdef HPCGAP + MEMBAR_READ(); +#endif + return result; } @@ -843,8 +847,7 @@ EXPORT_INLINE void SET_TYPE_COMOBJ(Obj obj, Obj val) { #ifdef HPCGAP MEMBAR_WRITE(); - // Only require a read guard - ((Obj *) CONST_ADDR_OBJ(obj))[0] = val; + UNSAFE_PTR_BAG(obj)[0] = val; #else ADDR_OBJ(obj)[0] = val; #endif @@ -880,7 +883,11 @@ EXPORT_INLINE BOOL IS_POSOBJ(Obj obj) */ EXPORT_INLINE Obj TYPE_POSOBJ(Obj obj) { - return CONST_ADDR_OBJ(obj)[0]; + Obj result = CONST_ADDR_OBJ(obj)[0]; +#ifdef HPCGAP + MEMBAR_READ(); +#endif + return result; } @@ -892,8 +899,7 @@ EXPORT_INLINE void SET_TYPE_POSOBJ(Obj obj, Obj val) { #ifdef HPCGAP MEMBAR_WRITE(); - // Only require a read guard - ((Obj *) CONST_ADDR_OBJ(obj))[0] = val; + UNSAFE_PTR_BAG(obj)[0] = val; #else ADDR_OBJ(obj)[0] = val; #endif @@ -929,7 +935,11 @@ EXPORT_INLINE BOOL IS_DATOBJ(Obj obj) */ EXPORT_INLINE Obj TYPE_DATOBJ(Obj obj) { - return CONST_ADDR_OBJ(obj)[0]; + Obj result = CONST_ADDR_OBJ(obj)[0]; +#ifdef HPCGAP + MEMBAR_READ(); +#endif + return result; } @@ -941,7 +951,12 @@ EXPORT_INLINE Obj TYPE_DATOBJ(Obj obj) */ EXPORT_INLINE void SET_TYPE_DATOBJ(Obj obj, Obj val) { +#ifdef HPCGAP + MEMBAR_WRITE(); + UNSAFE_PTR_BAG(obj)[0] = val; +#else ADDR_OBJ(obj)[0] = val; +#endif } void SetTypeDatObj(Obj obj, Obj type); From 60e052ae933de298c3fe1055cebc1cfa49cdecf4 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Fri, 7 Feb 2020 11:57:39 +0100 Subject: [PATCH 09/14] Add guards to some functions that modify object headers. --- src/boehm_gc.c | 6 +++++ src/gasman.h | 67 ++++++++++++++++++++++++++++++-------------------- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/boehm_gc.c b/src/boehm_gc.c index e82d020058..eb8b5576d6 100644 --- a/src/boehm_gc.c +++ b/src/boehm_gc.c @@ -334,6 +334,9 @@ void RetypeBagIntern(Bag bag, UInt new_type) if (old_type == new_type) return; +#if defined(HPCGAP) && defined(USE_HPC_GUARDS) + WriteGuard(bag); +#endif /* change the size-type word */ header->type = new_type; { @@ -456,6 +459,9 @@ UInt ResizeBag(Bag bag, UInt new_size) CollectBags(0, 0); #endif +#if defined(HPCGAP) && defined(USE_HPC_GUARDS) + WriteGuard(bag); +#endif BagHeader * header = BAG_HEADER(bag); /* get type and old size of the bag */ diff --git a/src/gasman.h b/src/gasman.h index f80ea5f57d..9be0d5a273 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -151,6 +151,46 @@ EXPORT_INLINE UInt TNUM_BAG(Bag bag) return CONST_BAG_HEADER(bag)->type; } +/**************************************************************************** +** +*F ReadCheck() . . . . . . . . . . . . . is the bag readable in HPCGAP? +*F WriteCheck() . . . . . . . . . . . . is the bag writable in HPCGAP? +*F HandleReadGuardError() . . . . . . . . . . signal read access error +*F HandleWriteGuardError() . . . . . . . . . signal write access error +** +** These funtion handle access checks in HPCGAP, i.e. whether a bag can +** safely be read or modified without causing race conditions. These are +** called in functions such as PTR_BAG() and CONST_PTR_BAG(), which should +** implicitly or explicitly guard all object accesses. In order to access +** objects without triggering checks, alternative functions UNSAFE_PTR_BAG() +** and UNSAFE_CONST_PTR_BAG() are provided. +*/ +#ifdef HPCGAP +extern int ExtendedGuardCheck(Bag) PURE_FUNC; + +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 ExtendedGuardCheck(bag); +} + +EXPORT_INLINE int WriteCheck(Bag bag) +{ + Region * region; + region = REGION(bag); + return !region || region->owner == GetTLS() || ExtendedGuardCheck(bag); +} + +extern void HandleReadGuardError(Bag) NORETURN; +extern void HandleWriteGuardError(Bag) NORETURN; +#endif // HPCGAP /**************************************************************************** ** @@ -310,33 +350,6 @@ 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 -extern int ExtendedGuardCheck(Bag) PURE_FUNC; - -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 ExtendedGuardCheck(bag); -} - -EXPORT_INLINE int WriteCheck(Bag bag) -{ - Region * region; - region = REGION(bag); - return !region || region->owner == GetTLS() || ExtendedGuardCheck(bag); -} - -extern void HandleReadGuardError(Bag) NORETURN; -extern void HandleWriteGuardError(Bag) NORETURN; -#endif // HPCGAP - EXPORT_INLINE Bag *PTR_BAG(Bag bag) { GAP_ASSERT(bag != 0); From 6055041374c726c41e3a6b504f9a8cad942a49c6 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Mon, 30 Mar 2020 12:01:00 +0200 Subject: [PATCH 10/14] Workaround to make the recog package load again. The recog package assigns to SMTX.InvariantBilinearForm in c6.gi; however, for HPCGAP, this record is made read-only. We change it to an atomic record so that we don't get a write guard error. --- lib/meatauto.gi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/meatauto.gi b/lib/meatauto.gi index 4815249aac..6eee9aabed 100644 --- a/lib/meatauto.gi +++ b/lib/meatauto.gi @@ -1690,5 +1690,5 @@ SMTX.SetEndAlgResidue:=SMTX.Setter("endAlgResidue"); SMTX.EndAlgResidue:=SMTX.Getter("endAlgResidue"); if IsHPCGAP then - MakeReadOnlyObj(SMTX); + SMTX := AtomicRecord(SMTX); fi; From e2bf918c4a813bfb7ae18c76a8d0049bac0497c1 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Tue, 21 Apr 2020 09:24:56 +0200 Subject: [PATCH 11/14] Make HELP_VIEWER_INFO an atomic record in HPC-GAP. This permits packages to add entries after initialization, while still allowing access from multiple threads. --- lib/helpview.gd | 6 +++++- lib/helpview.gi | 4 ---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/helpview.gd b/lib/helpview.gd index 47b32c0185..71f9af81b0 100644 --- a/lib/helpview.gd +++ b/lib/helpview.gd @@ -34,7 +34,11 @@ ## ## ## <#/GAPDoc> -BindGlobal("HELP_VIEWER_INFO", rec()); +if IsHPCGAP then + BindGlobal("HELP_VIEWER_INFO", AtomicRecord()); +else + BindGlobal("HELP_VIEWER_INFO", rec()); +fi; DeclareGlobalFunction("FindWindowId"); DeclareGlobalFunction("SetHelpViewer"); diff --git a/lib/helpview.gi b/lib/helpview.gi index 44f8063d42..b924b84880 100644 --- a/lib/helpview.gi +++ b/lib/helpview.gi @@ -432,10 +432,6 @@ show := function(file) end ); -if IsHPCGAP then - MakeReadOnlyObj(HELP_VIEWER_INFO); -fi; - ############################################################################# ## #F SetHelpViewer(): Set the viewer used for help From 2a660cb11714fbf0f27480040fc839fb4f4d85ca Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Tue, 21 Apr 2020 09:28:08 +0200 Subject: [PATCH 12/14] New workaround for dealing with PackageInfo records in HPC-GAP. In order to make PackageInfo records accessible from other threads, they need to be immutable, readonly, or atomic. Previously, they were made immutable, but that broke packages that altered them after initialization. The new workaround makes the package record itself atomic and its contents immutable. See issue #2568 for further discussion. --- lib/package.gi | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/package.gi b/lib/package.gi index bbab34779c..430c57e33f 100644 --- a/lib/package.gi +++ b/lib/package.gi @@ -242,11 +242,15 @@ BindGlobal( "AddPackageInfos", function( files, pkgdir, ignore ) record.PackageDoc:= [ record.PackageDoc ]; fi; if IsHPCGAP then - # FIXME: we make the package info record immutable, to - # allow access from multiple threads; but that in turn - # can break packages, which rely on their package info - # record being readable (see issue #2568) - MakeImmutable(record); + # FIXME: we make the package info record atomic and its + # entries immutable in order to allow for access from + # multiple threads. This is an iteration on a previous + # workaround related to issue #2568, where packages rely on + # being able to update packag info later. It can still break + # packages that either write thread-local information into + # this record that then get accessed from other threads or + # by concurrent access from multiple threads. + record := AtomicRecord(MakeReadOnlyObj(record)); fi; Add( GAPInfo.PackagesInfo, record ); fi; @@ -343,11 +347,11 @@ InstallGlobalFunction( InitializePackagesInfoRecords, function( arg ) record.( name ):= [ r ]; fi; if IsHPCGAP then - # FIXME: we make the package info record immutable, to + # FIXME: we make the package info record readonly, to # allow access from multiple threads; but that in turn # can break packages, which rely on their package info # record being readable (see issue #2568) - MakeImmutable( record.( name ) ); + MakeReadOnlyObj( record.( name ) ); fi; od; GAPInfo.PackagesInfo:= AtomicRecord(record); From 48177e26118ac557e130bef3a7067434dd866c25 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Tue, 13 Oct 2020 15:19:44 +0200 Subject: [PATCH 13/14] Increase default stack size for thread stacks. --- src/hpc/thread.c | 12 ++++++++++++ src/hpc/tlsconfig.h | 6 +++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/hpc/thread.c b/src/hpc/thread.c index 3ca209da35..7ed041a887 100644 --- a/src/hpc/thread.c +++ b/src/hpc/thread.c @@ -32,6 +32,10 @@ #include #include +#ifdef HAVE_SYS_RESOURCE_H +#include +#endif + #ifdef USE_BOEHM_GC # ifdef HPCGAP # define GC_THREADS @@ -323,6 +327,14 @@ void RunThreadedMain(int (*mainFunction)(int, char **), */ volatile int dummy[1]; size_t amount; +#ifdef HAVE_SYS_RESOURCE_H + // Ensure that we have enough room to allocate the stack + // on a boundary that is a multiple of TLS_SIZE. + struct rlimit rlim; + getrlimit(RLIMIT_STACK, &rlim); + rlim.rlim_cur = TLS_SIZE * 3; + setrlimit(RLIMIT_STACK, &rlim); +#endif amount = ((uintptr_t)dummy) & ~TLS_MASK; volatile char * p = alloca(((uintptr_t)dummy) & ~TLS_MASK); volatile char * q; diff --git a/src/hpc/tlsconfig.h b/src/hpc/tlsconfig.h index d2b6fad6ee..ee3867af68 100644 --- a/src/hpc/tlsconfig.h +++ b/src/hpc/tlsconfig.h @@ -21,7 +21,11 @@ #ifndef USE_NATIVE_TLS enum { - TLS_SIZE = (sizeof(UInt) == 8) ? (1 << 20) : (1 << 18), +#ifdef HAVE_SYS_RESOURCE_H + TLS_SIZE = (sizeof(UInt) == 8) ? (1 << 23) : (1 << 22), +#else + TLS_SIZE = (sizeof(UInt) == 8) ? (1 << 22) : (1 << 21), +#endif }; #define TLS_MASK (~(TLS_SIZE - 1)) From 8ade01f8e48f9ca13885b305f53028dd6358c592 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Tue, 13 Oct 2020 18:47:36 +0200 Subject: [PATCH 14/14] Drop ImpliedWriteGuard() from opers.cc --- src/opers.cc | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/opers.cc b/src/opers.cc index 034d2a6af3..113803fd11 100644 --- a/src/opers.cc +++ b/src/opers.cc @@ -1501,10 +1501,6 @@ static Obj FuncCOMPACT_TYPE_IDS(Obj self) // TYPE_OBJ static inline Obj TYPE_OBJ_FEO(Obj obj) { -#ifdef HPCGAP - /* TODO: We need to be able to automatically derive this. */ - ImpliedWriteGuard(obj); -#endif switch ( TNUM_OBJ( obj ) ) { case T_COMOBJ: return TYPE_COMOBJ(obj); @@ -2502,9 +2498,6 @@ static Obj WRAP_NAME(Obj name, const char *addon) UInt name_len = GET_LEN_STRING(name); UInt addon_len = strlen(addon); Obj fname = NEW_STRING( name_len + addon_len + 2 ); -#ifdef HPCGAP - ImpliedWriteGuard(fname); -#endif char *ptr = CSTR_STRING(fname); memcpy( ptr, addon, addon_len );