Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: New HPC-GAP guard checking without using ward #2845

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

WIP: New HPC-GAP guard checking without using ward #2845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
115687e
Make HPC-GAP guards work without Ward.
rbehrends Nov 15, 2019
ed182a5
Clean up HPC-GAP changes.
rbehrends Dec 2, 2019
61f7972
Remove implied guards.
rbehrends Feb 1, 2020
b550ef9
Check for --enable-guards/hpcgap consistency in configure.ac
rbehrends Feb 1, 2020
b6b175f
Fix guards for SET_TYPE_POSOBJ()/SET_TYPE_COMOBJ().
rbehrends Feb 3, 2020
2a63fb8
Remove unnecessary ReadGuard().
rbehrends Feb 5, 2020
b75b536
Reduce code overhead from guard checks.
rbehrends Feb 5, 2020
b29c2d4
Fix concurrent T_DATOBJ/T_COMOBJ/T_POSOBJ type access.
rbehrends Feb 7, 2020
60e052a
Add guards to some functions that modify object headers.
rbehrends Feb 7, 2020
6055041
Workaround to make the recog package load again.
rbehrends Mar 30, 2020
e2bf918
Make HELP_VIEWER_INFO an atomic record in HPC-GAP.
rbehrends Apr 21, 2020
2a660cb
New workaround for dealing with PackageInfo records in HPC-GAP.
rbehrends Apr 21, 2020
48177e2
Increase default stack size for thread stacks.
rbehrends Oct 13, 2020
8ade01f
Drop ImpliedWriteGuard() from opers.cc
rbehrends Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,29 @@ 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 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.
Expand Down Expand Up @@ -978,6 +1001,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],[
Expand Down
18 changes: 16 additions & 2 deletions etc/ci-prepare.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ BUILDDIR=$PWD

if [[ $HPCGAP = yes ]]
then
CONFIGFLAGS="--enable-hpcgap $CONFIGFLAGS"
CONFIGFLAGS="--enable-hpcgap --enable-guards $CONFIGFLAGS"
fi


Expand All @@ -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
fingolfin marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
6 changes: 5 additions & 1 deletion lib/helpview.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@
## </Description>
## </ManSection>
## <#/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");
Expand Down
4 changes: 0 additions & 4 deletions lib/helpview.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,6 @@ show := function(file)
end
);

if IsHPCGAP then
MakeReadOnlyObj(HELP_VIEWER_INFO);
fi;

#############################################################################
##
#F SetHelpViewer(<viewer>): Set the viewer used for help
Expand Down
2 changes: 1 addition & 1 deletion lib/meatauto.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1690,5 +1690,5 @@ SMTX.SetEndAlgResidue:=SMTX.Setter("endAlgResidue");
SMTX.EndAlgResidue:=SMTX.Getter("endAlgResidue");

if IsHPCGAP then
MakeReadOnlyObj(SMTX);
SMTX := AtomicRecord(SMTX);
fi;
18 changes: 11 additions & 7 deletions lib/package.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions src/boehm_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
{
Expand Down Expand Up @@ -456,6 +459,9 @@ UInt ResizeBag(Bag bag, UInt new_size)
CollectBags(0, 0);
#endif

#if defined(HPCGAP) && defined(USE_HPC_GUARDS)
WriteGuard(bag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't RetypeBag need this as well?

Also, shouldn't it be inside an #ifdef HPCGAP?

Copy link
Contributor Author

@rbehrends rbehrends Feb 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this push went through prematurely. I've been experimenting with various checks on the header functions. ResizeBag() seems to not cause any problems; but other functions — such as SIZE_BAG() — do when you add guards to them. This seems to be mostly related to some internal kernel objects, for reasons that I don't fully understand yet.

If I add checks to BAG_HEADER() directly, this seems to cause an overhead on the order of 10%, possibly a bit less (with clang).

#endif
BagHeader * header = BAG_HEADER(bag);

/* get type and old size of the bag */
Expand Down
73 changes: 73 additions & 0 deletions src/gasman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@

#include "common.h"

#ifdef HPCGAP
#include "hpc/region.h"
#include "hpc/tls.h"
#endif


/****************************************************************************
**
Expand Down Expand Up @@ -146,6 +151,46 @@ EXPORT_INLINE UInt TNUM_BAG(Bag bag)
return CONST_BAG_HEADER(bag)->type;
}

/****************************************************************************
**
*F ReadCheck(<bag>) . . . . . . . . . . . . . is the bag readable in HPCGAP?
*F WriteCheck(<bag>) . . . . . . . . . . . . is the bag writable in HPCGAP?
*F HandleReadGuardError(<bag>) . . . . . . . . . . signal read access error
*F HandleWriteGuardError(<bag>) . . . . . . . . . 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

/****************************************************************************
**
Expand Down Expand Up @@ -308,15 +353,43 @@ EXPORT_INLINE UInt SIZE_BAG_CONTENTS(const void *ptr)
EXPORT_INLINE Bag *PTR_BAG(Bag bag)
{
GAP_ASSERT(bag != 0);
#ifdef USE_HPC_GUARDS
if (!WriteCheck(bag))
HandleWriteGuardError(bag);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I never understood: Why don't we need some kind of read/write guard in BAG_HEADER as well? Right now, is there anything that would stop two threads from using ResizeBag/RetypeBag concurrently, possibly leading in a garbage output?

(This thought just motivated me to submit PR #3884).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are dealing with two cases here. One is TNUM_OBJ(), which we don't want to burden with additional guard checks, as it's used all the time. Changes are generally either harmless or in the rare cases where they do matter, handled explicitly. The biggest unpleasantness here is KTNumPlist(), which has an explicit guard check for this reason.

You are correct that with resizing read-only objects, we run into a problem. I believe the code has assumed that the functions that resize objects (such as GROW_PLIST() would fail naturally when changing the length word in the object). However, it looks like sometimes the change happens rather late.

I'd still simply add a write guard to ResizeBag(), where it causes minimal overhead, rather than making all accesses to the header more expensive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I don't want to burden TNUM_BAG, SIZE_BAG, TEST_OBJ_FLAGS with guards; but they can be written to, and so at least RetypeBage / ResizeBag and perhaps also the SET/CLEAR_OBJ_FLAGS functions should have guards... I always wondered why they didn't.

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);
Expand Down
2 changes: 0 additions & 2 deletions src/gvars.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
Loading