Here are the two patches as applied in sage. The patch 0002 matches the merged
pull request 3043 and 0003 the first two commits of the pull request 3096.
Best,
Tobias
On 12/19/18 8:41 PM, Bill Allombert wrote:
> On Tue, Dec 18, 2018 at 12:20:00PM +0100, Tobias Hansen wrote:
>> On 12/17/18 10:28 PM, Bill Allombert wrote:
>>>>> Does that answer your questions ?
>>>>>
>>>>>> + * Install libgap to /usr/lib/triplet.
>>>>> Do you need this now ? When the interface to libgap has stabilized, then
>>>>> probably we will split libgap from gap-dev and move it to
>>>>> /usr/lib/triplet, but as long as it is an experimental feature it is
>>>>> best to keep it in a subdirectory.
>>>> I would prefer not having to apply yet another workaround to find this
>>>> library. It is possible though.
>>> Part of the issue is that we can little afford to go to the NEW queue to add
>>> extra binary packages if we want to be ready before the freeze.
>>> What do you suggest ?
>> If that saves us from going through NEW, I'm ok with leaving libgap in
>> usr/lib/triplet/gap for now (assuming that we can get it to work that way).
>>
>>>> Would you be ok with applying these two patches?
>>> I do not like the idea of applying SAGE-specific patches to /usr/bin/gap.
>>>
>>> However if we arrange for the patch to be applied only to libgap, then I
>>> am ready to apply whatever you need.
>>>
>>> Cheers,
>> Apart from libgap-api.h the two patches also add some lines to
>> error.h, gapstate.h and gasman.h. How can we proceed? By adding a
>> second copy of all headers to /usr/include/libgap in gap-dev?
> Well it seems at least 3043 will be merged for GAP 4.10.1, so maybe
> there is no need for /usr/include/libgap etc.
>
> Could you send me the latest versions of the two patches as separate
> attachments,
> not as an interdiff ?
>
> Cheers,
>From 0cecb79ff97c73a24acacf8afdc3edba93507661 Mon Sep 17 00:00:00 2001
From: "Erik M. Bray" <erik.b...@lri.fr>
Date: Thu, 22 Nov 2018 10:53:31 +0100
Subject: [PATCH 2/3] kernel: add helper function for writing error messages to
the file/stream referenced by the ERROR_OUTPUT global variable
---
src/error.c | 42 ++++++++++++++++++++++++++++++++++++++++++
src/error.h | 8 ++++++++
src/scanner.c | 3 ++-
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/src/error.c b/src/error.c
index 9bb3be8..d43347d 100644
--- a/src/error.c
+++ b/src/error.c
@@ -33,6 +33,8 @@
static Obj ErrorInner;
+static Obj ERROR_OUTPUT = NULL;
+static Obj IsOutputStream;
/****************************************************************************
@@ -40,6 +42,44 @@ static Obj ErrorInner;
*F * * * * * * * * * * * * * * error functions * * * * * * * * * * * * * * *
*/
+/****************************************************************************
+**
+*F OpenErrorOutput() . . . . . . . open the file or stream assigned to the
+** ERROR_OUTPUT global variable defined in
+** error.g, or "*errout*" otherwise
+*/
+UInt OpenErrorOutput( void )
+{
+ /* Try to print the output to stream. Use *errout* as a fallback. */
+ UInt ret = 0;
+
+ if (ERROR_OUTPUT != NULL) {
+ if (IsStringConv(ERROR_OUTPUT)) {
+ ret = OpenOutput(CSTR_STRING(ERROR_OUTPUT));
+ }
+ else {
+ if (CALL_1ARGS(IsOutputStream, ERROR_OUTPUT) == True) {
+ ret = OpenOutputStream(ERROR_OUTPUT);
+ }
+ }
+ }
+
+ if (!ret) {
+ /* It may be we already tried and failed to open *errout* above but
+ * but this is an extreme case so it can't hurt to try again
+ * anyways */
+ ret = OpenOutput("*errout*");
+ if (ret) {
+ Pr("failed to open error stream\n", 0, 0);
+ }
+ else {
+ Panic("failed to open *errout*");
+ }
+ }
+
+ return ret;
+}
+
/****************************************************************************
**
@@ -615,6 +655,8 @@ static Int InitKernel(StructInitInfo * module)
InitHdlrFuncsFromTable(GVarFuncs);
ImportFuncFromLibrary("ErrorInner", &ErrorInner);
+ ImportFuncFromLibrary("IsOutputStream", &IsOutputStream);
+ ImportGVarFromLibrary("ERROR_OUTPUT", &ERROR_OUTPUT);
// return success
return 0;
diff --git a/src/error.h b/src/error.h
index 31af256..1f5ee5d 100644
--- a/src/error.h
+++ b/src/error.h
@@ -32,6 +32,14 @@ Int RegisterBreakloopObserver(intfunc func);
/****************************************************************************
**
+*F OpenErrorOutput() . . . . . . . open the file or stream assigned to the
+** ERROR_OUTPUT global variable defined in
+** error.g, or "*errout*" otherwise
+*/
+extern UInt OpenErrorOutput();
+
+/****************************************************************************
+**
*F ErrorQuit( <msg>, <arg1>, <arg2> ) . . . . . . . . . . . print and quit
*/
extern void ErrorQuit(const Char * msg, Int arg1, Int arg2) NORETURN;
diff --git a/src/scanner.c b/src/scanner.c
index 4db17b3..071c0e3 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -16,6 +16,7 @@
#include "scanner.h"
+#include "error.h"
#include "gapstate.h"
#include "gaputils.h"
#include "io.h"
@@ -42,7 +43,7 @@ static void SyntaxErrorOrWarning(const Char * msg, UInt error)
if (STATE(NrErrLine) == 0) {
// open error output
- OpenOutput("*errout*");
+ OpenErrorOutput();
// print the message ...
if (error)
--
1.9.1
>From 798756448180195a6ce020565a5c1d160e491e98 Mon Sep 17 00:00:00 2001
From: "Erik M. Bray" <erik.b...@lri.fr>
Date: Thu, 6 Dec 2018 16:11:35 +0000
Subject: [PATCH 3/3] Prototype for 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.
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 _MarkStackBottomBags which just updates
the StackBottom variable. Then the macro called MarkStackBottomBags (same name
without underscore) 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 state variable STATE(EnterStackCount) 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(). Some functions are provided for
manipulating the counter from the API without directly exposing the GAP state,
but I'm not sure if this is necessary or desirable, especially since it means
EnterStackCount isn't updated atomically. My hope was to avoid exposing too
many GAP internals, but it may be necessary in order to implement these as
macros.
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.
Conflicts:
src/libgap-api.h
---
src/gapstate.h | 3 ++
src/gasman.c | 6 ++++
src/gasman.h | 15 ++++++++++
src/libgap-api.c | 27 +++++++++++++++++
src/libgap-api.h | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
5 files changed, 137 insertions(+), 2 deletions(-)
diff --git a/src/gapstate.h b/src/gapstate.h
index 72ec4c3..7a2a663 100644
--- a/src/gapstate.h
+++ b/src/gapstate.h
@@ -97,6 +97,9 @@ typedef struct GAPState {
UInt1 StateSlots[STATE_SLOTS_SIZE];
+ /* For libgap-api.c */
+ Int EnterStackCount;
+
/* Allocation */
#if !defined(USE_GASMAN)
#define MAX_GC_PREFIX_DESC 4
diff --git a/src/gasman.c b/src/gasman.c
index 13c0b1e..4d2ab3d 100644
--- a/src/gasman.c
+++ b/src/gasman.c
@@ -1193,6 +1193,12 @@ void SetExtraMarkFuncBags(TNumExtraMarkFuncBags func)
}
+
+void _MarkStackBottomBags(void* StackBottom) {
+ StackBottomBags = StackBottom;
+}
+
+
void InitBags (
UInt initial_size,
Bag * stack_bottom,
diff --git a/src/gasman.h b/src/gasman.h
index 236eb8b..55e057a 100644
--- a/src/gasman.h
+++ b/src/gasman.h
@@ -982,6 +982,21 @@ extern void InitCollectFuncBags (
typedef void (*TNumExtraMarkFuncBags)(void);
extern void SetExtraMarkFuncBags(TNumExtraMarkFuncBags func);
+
+#ifdef __GNUC__
+#define MarkStackBottomBags() \
+ _MarkStackBottomBags(__builtin_frame_address(0));
+/*
+#else
+ * TODO: Detect the best stack frame detection technique at configure time
+ *
+#define MarkStackBottomBags() \
+ register void* rbp asm("rbp"); \
+ _MarkStackBottomBags(rbp);
+*/
+#endif
+extern void _MarkStackBottomBags(void* StackBottom);
+
/****************************************************************************
**
*F InitBags(...) . . . . . . . . . . . . . . . . . . . . . initialize Gasman
diff --git a/src/libgap-api.c b/src/libgap-api.c
index 82cc441..e75b0e2 100644
--- a/src/libgap-api.c
+++ b/src/libgap-api.c
@@ -10,6 +10,8 @@
#include "lists.h"
#include "streams.h"
#include "stringobj.h"
+#include "system.h"
+
//
// Setup and initialisation
@@ -60,3 +62,28 @@ Obj GAP_EvalString(const char * cmd)
res = READ_ALL_COMMANDS(instream, False, True, viewObjFunc);
return res;
}
+
+inline syJmp_buf * _GAP_GetReadJmpError(void)
+{
+ return &(STATE(ReadJmpError));
+}
+
+inline Int _GAP_GetEnterStackCount(void)
+{
+ return STATE(EnterStackCount);
+}
+
+inline void _GAP_IncEnterStackCount(void)
+{
+ STATE(EnterStackCount)++;
+}
+
+inline void _GAP_DecEnterStackCount(void)
+{
+ STATE(EnterStackCount)--;
+}
+
+inline void _GAP_SetEnterStackCount(Int count)
+{
+ STATE(EnterStackCount) = count;
+}
diff --git a/src/libgap-api.h b/src/libgap-api.h
index e45d6fc..55fcd05 100644
--- a/src/libgap-api.h
+++ b/src/libgap-api.h
@@ -5,9 +5,93 @@
#include "gap.h"
-typedef void (*CallbackFunc)(void);
+#ifdef __GNUC__
+#define unlikely(x) __builtin_expect(!!(x), 0)
+#else
+#define unlikely(x) (x)
+#endif
+
+
+#ifndef GAP_ENTER_DEBUG
+#define GAP_ENTER_DEBUG 0
+#endif
+
+
+extern syJmp_buf * _GAP_GetReadJmpError(void);
+extern Int _GAP_GetEnterStackCount(void);
+extern void _GAP_IncEnterStackCount(void);
+extern void _GAP_DecEnterStackCount(void);
+extern void _GAP_SetEnterStackCount(Int count);
+
+
+#if GAP_ENTER_DEBUG
+#define GAP_ENTER_DEBUG_MESSAGE(message, file, line) \
+ fprintf(stderr, "%s: %d; %s:%d\n", message, _GAP_EnterStackCount, file, line);
+#else
+#define GAP_ENTER_DEBUG_MESSAGE(message, file, line)
+#endif
+
+
+#define GAP_EnterStack() \
+ GAP_ENTER_DEBUG_MESSAGE("EnterStack", __FILE__, __LINE__); \
+ Int _gap_tmp_enter_stack_count = _GAP_GetEnterStackCount(); \
+ if (_gap_tmp_enter_stack_count < 0) { \
+ _GAP_SetEnterStackCount(-_gap_tmp_enter_stack_count); \
+ } else { \
+ if (_gap_tmp_enter_stack_count == 0) { \
+ MarkStackBottomBags(); \
+ } \
+ _GAP_IncEnterStackCount(); \
+ }
+
-// Initialisation and finalization
+#define GAP_LeaveStack() \
+ _GAP_DecEnterStackCount(); \
+ GAP_ENTER_DEBUG_MESSAGE("LeaveStack", __FILE__, __LINE__);
+
+
+static inline int _GAP_Error_Prejmp(const char* file, int line) {
+#if GAP_ENTER_DEBUG
+ GAP_ENTER_DEBUG_MESSAGE("Error_Prejmp", file, line);
+#endif
+ if (_GAP_GetEnterStackCount() > 0) {
+ return 1;
+ }
+ return 0;
+}
+
+
+static inline int _GAP_Error_Postjmp(int JumpRet)
+{
+ if (unlikely(JumpRet != 0)) {
+ /* 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 */
+ Int tmp_count = _GAP_GetEnterStackCount();
+ if (tmp_count > 0) {
+ _GAP_SetEnterStackCount(-tmp_count);
+ }
+ return 0;
+ }
+
+ return 1;
+}
+
+#define GAP_Error_Setjmp() (_GAP_Error_Prejmp(__FILE__, __LINE__) || \
+ _GAP_Error_Postjmp(sySetjmp(*_GAP_GetReadJmpError())))
+
+
+#define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
+#define GAP_Leave() GAP_LeaveStack()
+
+
+////
+//// Setup and initialisation
+////
+typedef void (*CallbackFunc)(void);
void GAP_Initialize(int argc,
char ** argv,
--
1.9.1
