Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions src/gasman.c
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,13 @@ void SetExtraMarkFuncBags(TNumExtraMarkFuncBags func)

GAP_STATIC_ASSERT((sizeof(BagHeader) % sizeof(Bag)) == 0, "BagHeader size must be multiple of word size");


void SetStackBottomBags(void * StackBottom)
{
StackBottomBags = StackBottom;
}


void InitBags (
UInt initial_size,
Bag * stack_bottom,
Expand Down
2 changes: 2 additions & 0 deletions src/gasman.h
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,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
Expand Down
66 changes: 66 additions & 0 deletions src/libgap-api.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@

// LibGAP API - API for using GAP as shared library.

#include <signal.h>

#include "libgap-api.h"

#include "ariths.h"
#include "bool.h"
#include "calls.h"
#include "gap.h"
#include "gapstate.h"
#include "gasman.h"
#include "gvars.h"
#include "integer.h"
#include "lists.h"
Expand All @@ -26,6 +29,7 @@
#include "streams.h"
#include "stringobj.h"


//
// Setup and initialisation
//
Expand Down Expand Up @@ -346,3 +350,65 @@ Obj GAP_CharWithValue(UChar obj)
{
return ObjsChar[obj];
}

syJmp_buf * GAP_GetReadJmpError(void)
{
return &(STATE(ReadJmpError));
}


static volatile sig_atomic_t EnterStackCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be sig_atomic_t? Which signal handler is calling code modifying this variable? If none, revert to int; otherwise, please add a comment explaining it (can be brief)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None specifically, but my concern here is that one could: E.g. a signal handler installed by GAP which runs and results in an error, which can in turn result in running code that does modify this variable. I could try to concoct a real example, but something like that seems plausible.

Copy link
Member

Choose a reason for hiding this comment

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

GAP never installs such a signal handler, and indeed, calling any kind of GAP code from a signal handler would be very dangerous and is unlikely to work.

The reason I am asking is not that I want to pain the bikeshed; but rather it is that using sig_atomic_t IMHO adds considerable to the complexity of this code. If I or somebody else need to edit that code, I may wonder "but why is it using sig_atomic_t?", and not knowing the answer means that I don't understand the code, and hence better not touch it; or touch it but with a bad feeling that I might be breaking something.

This is why I ask whether it is really necessary, and if so, that a comment be added explaining it. Even if that comment says something like "// It is unclear whether we need sig_atomic_t here, but just out of paranoia, we use it anyway".

That said, I'd still prefer if it was not used; I honestly can't think of any example where it might be used that is not horribly unsafe anyway. So if you can think of any, I'd be most interested to hear about it, even if it is just a rough sketch.

The argument that one "could" need it seems week to me: with that argument, perhaps we should also add a mutex protecting access to EnterStackCount (or require C11 atomics, or something like that)? After all, multiple threads might want to use the GAP API. Sure, it's unlikely that this can work, but one could try it anyway.... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure on what basis you can assert that it "adds considerable...complexity". Exactly what complexity is it adding? sig_atomic_t is standard POSIX, and all it means is that it's an instruction to the compiler to only manipulate this variable with single instructions. Furthermore, according to the GCC docs, "In practice, you can assume that int is atomic...these assumptions are true on all of the machines that the GNU C Library supports and on all POSIX systems we know of."

It's really just a guarantee that if this code happens to be interrupted by a signal (with a signal handler that returns) it will leave things in a sane state. For a signal handler that doesn't return--i.e. exits the process or longjumps to somewhere arbitrary) you can't make any further guarantees, but at the very least you can safely increment and decrement it even if a signal occurs. That's all.

Choose a reason for hiding this comment

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

at the very least you can safely increment and decrement it even if a signal occurs. That's all.

Just to clarify this: the type sig_atomic_t means that it can be read from atomically and written to atomically. Any operation more complicated than that (such as an increment/decrement) may not be atomic. For example, the operations EnterStackCount = -EnterStackCount; and EnterStackCount++; may not be atomic.

So I actually agree with @fingolfin here: better not make any guarantees about atomicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True--in that case it's probably best avoided. I know in Cysignals itself sig_block()/unblock() just set a flag that determines what to do in the actual signal handler should a signal occur. But in the case of libgap we want to leave most details of signal handling up to the embedding application, so it would be tricky to implement something like this in a way that can be sensibly embedded in arbitrary applications. So perhaps it's best to just keep it simple.

Regarding your idea, I'm not saying it can't be done. I just don't understand exactly what you're proposing. Keep in mind these macros should be able to work properly recursively. Though the exact value of EnterStackCount is less important than just tracking whether or not we're in a recursive call.

Choose a reason for hiding this comment

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

Obvious question: do we really need to support nesting for these macros? It would certainly simplify things if we could say that nesting is not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory one could write code such that they never have to be nested. For example there are cases where we could refactor a function some_func_that_uses_gap() into some_func_that_uses_gap_outer() and `some_func_that_uses_gap_inner(), where the latter can be called directly in other code that is already bracketed by the Enter/Leave macros.

So technically it's possible. But it does put a higher strain on the user in exactly how they structure their code, and it's ugly (imagine if sig_on and sig_off could not be nested). You would also have to actively prevent the user from nesting them somehow if it did not work (otherwise this would lead to hard to trace bugs). I think it's better to allow nesting. Any complication that arises from that seems better and less probable than not allowing it.

Choose a reason for hiding this comment

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

For now, I propose a simple and concrete solution:

  • In GAP, don't try to make these macros signal-safe. Do you know about other applications using libGAP which also need to handle signals?
  • In SageMath, wrap GAP_Enter/GAP_Leave in sig_block/sig_unblock calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which is essentially the status quo (we don't have the Sage part yet but that's easy, and I agree with it). So I'm fine with that for lack of a better solution.

Perhaps at most this should be documented, and left to individual users to worry about. E.g. for the GAP-Julia interface something might want to be done, but it would be just as Julia-specific as Cysignals is Python-specific. Other applications may not care as much about handling signals at all.



// 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;
}
}
128 changes: 128 additions & 0 deletions src/libgap-api.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,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
////
Expand Down
22 changes: 13 additions & 9 deletions tst/testlibgap/common.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,20 @@
void test_eval(const char * cmd)
{
Obj res, ires;
Int rc, i;
Int rc, i, ok;
printf("gap> %s\n", cmd);
res = GAP_EvalString(cmd);
rc = GAP_LenList(res);
for (i = 1; i <= rc; i++) {
ires = GAP_ElmList(res, i);
if (GAP_ElmList(ires, 1) == GAP_True) {
Char * buffer = GAP_CSTR_STRING(GAP_ElmList(ires, 5));
if (buffer)
printf("%s\n", buffer);
ok = GAP_Enter();
if (ok) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I wonder if the generic fallback code for GAP_EnterStack will work in this scenario? After all, we only reference the location of that particular stack variable. But we don't bother walking back to the start of the stack of the current function. So e.g. res could be outside the stack range considered by the GASMAN stack scanner, couldn't it? In which case there'd be a risk of a crash again.

To test this, I'd disable the GNU C version for it, and then change the loop below to perform some allocations and trigger garbage collections explicitly (we may need to expose a GAP_GC(int full) function for that; it would also be useful for some other uses, I believe).

Anyway, the "fix" in calling code would be to move all local variables storing GAP object references to inside the block between GAP_Enter and GAP_Leave (and to explicitly document this, too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you might be right. Do you even know off the top of your head (I'm not sure) if there is any guarantee whatsoever that there is any relation between the order some local variables are declared in C code, and what order they are placed on the stack (of course it could use registers for some of them too in which case it's irrelevant)? AFAIK the C standard doesn't even know anything about a "stack" as that's a platform-specific implementation detail. But realistically speaking this discussion is only relevant where there is one.

So I think there's only so much we can do about this--it's just a best guess but not fool-proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we already talked about exposing CollectBags in #3030.

It's not exactly clear to me what you want me to do here though. Just manually test this case, or actually update the test to test this case in general somehow (e.g. have a copy of these tests that are compiled in such a way that forces the fallback)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The C standard definately puts no requirements on the order of variables on the stack, and will mix them up however it likes. If you are capturing a reference to a local variable in your fallback, the old remotely safe option would be to call GAP_enter, and then call a non-inlined function to actually do the work.

I'd probably just not put in a fallback which captures a reference to a local variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I'm 100% positive that fallback is going to lead to hard-to-track and horrible crashes, if there are any GAP variables in the current function scope, or in any inlined functions.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and of course we don't expect you, @embray , to provide fallbacks fo "every imaginable architecture". My stance on that is that it's better to wait for somebody who actually uses those architectures anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in practice most compilers do seem to allocate locals in order of definition on the stack

I have noted this as well, at least with gcc. That's why I thought it was an "acceptable" fallback in the first place.

but taking an address of it and passing it to a non-inline function should avoid that

It does, again at least in the case of gcc (and actually I can't think of any other reasonable thing for any compiler to do in that case: how are you supposed to pass a pointer to a value in a register?) And of course, for locals that the compiler puts in registers this is a moot point (GASMAN handles that case separately with its setjmp hack).

So indeed, it seems best to remove the generic fallback, and wait for people to complain that their favorite compiler does not work anymore with it.

+1 I don't like it, but it does seem like the safest option for now, alas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW Boehm GC does use a similar fallback (they also declare the dummy variable volatile, which I intended to do as well but forgot): https://github.com/ivmai/bdwgc/blob/c50de12ab045b57953e152545836c23f76e7bc24/mark_rts.c#L502
It also implements a GC_get_stack_base() for quite a number of different platforms, that I think is also supposed to work properly on per-thread stcks.

they would also break GAP by itself. If this ever happens, we'll have to go back to the drawing board anyway

I think that may be the case anyways. Is there really an advantage to continuing to maintain a separate GC when something more heavily used and robust like Boehm can be used? Or at least, maybe, a customized fork thereof if there's really some fine-tuning necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

gasman is much faster than Boehm, and various people's attempt to tune it haven't closed the gap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be worth at least borrowing bits from, such as those linked above, where applicable (and assuming license compatibility). It would be great, longer-term, to get rid of this EnterStack/LeaveStack stuff entirely.

res = GAP_EvalString(cmd);
rc = GAP_LenList(res);
for (i = 1; i <= rc; i++) {
ires = GAP_ElmList(res, i);
if (GAP_ElmList(ires, 1) == GAP_True) {
Char * buffer = GAP_CSTR_STRING(GAP_ElmList(ires, 5));
if (buffer)
printf("%s\n", buffer);
}
}
}
GAP_Leave();
}