Factor out state

[markuspf]

This pull request is a work-in-progress on the unification of GAP and HPC-GAP.

It takes the approach to remove a lot of globally defined state and put it into a structure currently called InterpreterState. Most of the code is taken from the hpcgap-default branch.

This compiles, but immediately segfaults (because of missing initialisation code which I will add next).

This pull request is to be understood as a base for discussion at the moment.

Obviously the first goal is to get GAP and HPC-GAP closer to each other: We mostly copy code from HPC-GAP, but just get rid of state stored in global variables. This is something that is desirable for GAP as well, though we might have a careful think about names (TLS as in ThreadLocalStorage might not be the ideal choice of name), and whether on the long run we want to have a state-struct per module or not.

[fingolfin]

This TL: prefix is rather cryptic. How about we consistently write something like:

/* Obj IntrResult;  -- definition has been moved to InterpreterState */

or so? That would help people trying to understand the code.

[markuspf]

This still segfaults. I gather this has something to do with the garbage collector state being messed up, but I haven't found out yet where this happens. If @ChrisJefferson has a quick insight that'd be appreciated. I'll continue hacking at it for now, maybe I'll understand gasman then.

[ChrisJefferson]

Two issues. One is easy to fix (need some InitGlobalBag), the other I'm not 100% sure what's going wrong (bags being created at wrong point) but it's easy to fix by calling InitPrintObjStack in InitKernel.

Patch follows (as I'm too lazy to push)

diff --git a/src/objects.c b/src/objects.c
index a309c6d..463bab2 100644
--- a/src/objects.c
+++ b/src/objects.c
@@ -865,17 +865,17 @@ static UInt LastPV = 0; /* This variable contains one of the values
                           PrintObj or ViewObj still open (0), or the
                           innermost such is Print (1) or View (2) */

-/* On-demand creation of the PrintObj stack */
+
 void InitPrintObjStack()
 {
-  if (!TLS(PrintObjThiss)) {
-    TLS(PrintObjThissObj) = NewBag(T_DATOBJ, MAXPRINTDEPTH*sizeof(Obj)+sizeof(Obj));
-    TLS(PrintObjThiss) = ADDR_OBJ(TLS(PrintObjThissObj))+1;
-    TLS(PrintObjIndicesObj) = NewBag(T_DATOBJ, MAXPRINTDEPTH*sizeof(Int)+sizeof(Obj));
-    TLS(PrintObjIndices) = (Int *)(ADDR_OBJ(TLS(PrintObjIndicesObj))+1);
-  }
+  InitGlobalBag(&TLS(PrintObjThissObj), "objects.c:PrintObjThissObj");
+  InitGlobalBag(&TLS(PrintObjIndicesObj), "objects.c:PrintObjIndicesObj");
+  TLS(PrintObjThissObj) = NewBag(T_DATOBJ, MAXPRINTDEPTH*sizeof(Obj)+sizeof(Obj));
+  TLS(PrintObjThiss) = ADDR_OBJ(TLS(PrintObjThissObj))+1;
+  TLS(PrintObjIndicesObj) = NewBag(T_DATOBJ, MAXPRINTDEPTH*sizeof(Int)+sizeof(Obj));
+  TLS(PrintObjIndices) = (Int *)(ADDR_OBJ(TLS(PrintObjIndicesObj))+1);
 }
-
+
 void            PrintObj (
     Obj                 obj )
 {
@@ -905,7 +905,6 @@ void            PrintObj (
     /* if <obj> is a subobject, then mark and remember the superobject
        unless ViewObj has done that job already */

-    InitPrintObjStack();
     if ( !fromview  && 0 < TLS(PrintObjDepth) ) {
         if ( IS_MARKABLE(TLS(PrintObjThis)) )  MARK( TLS(PrintObjThis) );
         TLS(PrintObjThiss)[TLS(PrintObjDepth)-1]   = TLS(PrintObjThis);
@@ -1028,7 +1027,6 @@ void            ViewObj (

     /* if <obj> is a subobject, then mark and remember the superobject     */

-    InitPrintObjStack();

     if ( 0 < TLS(PrintObjDepth) ) {
         if ( IS_MARKABLE(TLS(PrintObjThis)) )  MARK( TLS(PrintObjThis) );
@@ -1794,7 +1792,9 @@ static Int InitKernel (
     MakeImmutableObjFuncs[ T_POSOBJ ] = MakeImmutablePosObj;
     MakeImmutableObjFuncs[ T_DATOBJ ] = MakeImmutableDatObj;

-
+
+    InitPrintObjStack();
+
     /* return success                                                      */
     return 0;
 }

[markuspf]

Mhm. There I probably tried doing 2 steps at the same time (and finding again that it's a bad idea).

Did you use some magic incantation to find this, or does your system just give you better behaviour than "haha I am going to segfault randomly"?

[markuspf]

Also the current state passes testinstall & testbugfix for me. Running teststandard now.

[ChrisJefferson]

Nothing magic, just guessed the problem would be the rearrangement made the GC lose track of something, then scanned through the patch.

[markuspf]

Ah damn, I see what was wrong with my ported HPC-GAP code now I think. Thanks for the pointer, @ChrisJefferson

[markuspf]

Workspaces seem to be broken though (witnessed by the manuals not building)

[ChrisJefferson]

Looking more carefully, the whole PrintObjThiss and PrintObjIndices are broken. You can't store a pointer to a GAP object over a garbage collection!

The easiest option would be to just pop the C arrays you are removing into the struct, rather than replacing them with GAP objects.

[markuspf]

Note that this is code that I (blindly and stupidly) copied over from HPC-GAP. I'll pay more attention to this stuff in the future. This now passes tests and builds the documentation on my machine. I'll have another look at the differences now.

I keep thinking that every module should be able to register its own (global)state with the kernel (obviously without using global variables). With Boehm-GC we could just malloc the state, but with GASMAN we have to me more careful.

[markuspf]

@fingolfin @ChrisJefferson this builds now and passes tests. I am not sure how daring we are with this, but if we want the unification to happen before 4.9 we should probably merge things early and detect problems by using the code.

As I commented above, changing names to be more sensible, and rearranging things is in my opinion something we should keep a note of and do post-unification. (Even though for example changing the TLS() macro is of course a search-and-replace job, but one that one has to do on two branches as opposed to one.

[ChrisJefferson]

While parts of this patch are a little horrible, my temptation is to merge it, and head towards complete merging. Once that happens, we can then do all kinds of cleanups.

[olexandr-konovalov]

Weekly tests of the master branch are passing well today, so this could be a good moment to merge this and clean up any problems that may be discovered.

[olexandr-konovalov]

OK, so let's now merge it and see what happens.