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.

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

Add GAP kernel API (aka libgap) #2702

Merged
merged 1 commit into from
Aug 29, 2018
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,6 @@ doc/gapmacrodoc.idx
/hpcgap/ward

/builds/

/libgap.la
/.libs
3 changes: 3 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ matrix:
# test error reporting and compiling (quickest job in this test suite)
- env: TEST_SUITES="testspecial test-compile"

# test libgap
- env: TEST_SUITES="testlibgap"

script:
- gcov --version
- bash etc/ci-prepare.sh && bash etc/ci.sh
Expand Down
19 changes: 14 additions & 5 deletions Makefile.rules
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
all: gap$(EXEEXT) gac
.PHONY: all

libgap: libgap.la
.PHONY: libgap

# Backwards compatibility: add "default" target as alias for "all"
default: all
.PHONY: default
Expand Down Expand Up @@ -64,6 +61,9 @@ SOURCES += src/intfuncs.c
SOURCES += src/intrprtr.c
SOURCES += src/io.c
SOURCES += src/iostream.c
ifeq ($(HPCGAP),no) # we don't support a kernel API in HPC-GAP atm
SOURCES += src/libgap-api.c
endif
SOURCES += src/listfunc.c
SOURCES += src/listoper.c
SOURCES += src/lists.c
Expand Down Expand Up @@ -121,7 +121,6 @@ ifeq ($(HPCGAP),yes)
SOURCES += src/hpc/traverse.c
endif


########################################################################
# Preprocessor flags
#
Expand Down Expand Up @@ -389,13 +388,16 @@ gap$(EXEEXT): libgap.la cnf/GAP-LDFLAGS cnf/GAP-LIBS cnf/GAP-OBJS

else

all: libgap.so
libgap.so: symlinks libgap.la
$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) -shared $(OBJS) $(GAP_LIBS) -o $@
Copy link
Member

Choose a reason for hiding this comment

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

Disclaimer: the following is not meant to block this PR as such, just tries to clarify some background.

When I just looked at the above lines, I wondered again about the relation between libgap.so and libgap.la. There is now a dependency here -- but there seems no strict technical reason.

Moreover, on Mac OS X, the suffix .so is wrong, it should be .dylib. And of course on Windows one would expect gap.dll or so. But on Windows, we technically already build GAP into a DLL, as we need that, in order to be able to support kernel extensions at all.

However, all this has an "easy" solution: libgap.la already builds a shared library. That's what libtool .la files are for: they abstract over static/shared libraries in a portable way. Building libgap.la creates .libs/libgap.dylib for me on Mac OS X. Specifically:

$ ls .libs/
libgap.0.dylib  libgap.dylib  libgap.la  libgap.lai

So it already takes care of things like file extensions.

We abuse this for the Cygwin builds:

bin/$(GAPARCH)/gap.dll: symlinks libgap.la
	cp .libs/cyggap-0.dll $@  # FIXME: HACK to support kernel extensions

But that' a bad HACK, which I only added to make things work quickly. In reality, one should not directly access files in .libs, but rather use the libtool script to use these files, e.g. to link against them, or to install them.

Indeed, installing a .la may cause libtool to relink it (because the rpaths and other settings that one needs to use the library may differ between when using it inside a build directory during development; and when using it as a system wide installed library).

This all means that this libgap.so target should be temporary at best. What we should do instead, though, depends a bit on how libgap.so is supposed to be used: are people right now linking against it by specifying -L/path/to/GAPROOT -lgap? If so, and if they are using GNU libtool as well, they are better of specifying /path/to/GAPROOT/libgap.la. If so, but they are not using GNU libtool, it becomes a bit tricky.
Of course, if instead one requires people to do a make install from within GAP (which is not fully implemented, in particular the libgap part...), then all that goes away...

All in all, the best long term solution probably is to finally implement make install, or at least enough of it to make libgap usable by clients. I always planned to work on that eventually (but demand wasn't exactly high), and would be happy to collaborate on it, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I use libgap at the moment is by

(define-ffi-definer define-gap
  (ffi-lib "/home/makx/git/gap/master/.libs/libgap.so"))

in Racket (well knowing that this is of course at best a hack, too). If libgap.so were installed, I'd probably be able to just use `ffi-lib "gap".

I do not require header files, but I have to give GAP_Initialize the path to the GAP library. I believe this is mostly how SageMath uses libgap as well.

The reason the .so target is there is because I apparently still did not understand libtool. I did remember that the .la file is all we need, and that the .so file just appeared for me in .libs.

Happy to learn how to do the make install so that enough people are happy (but for that we need people to use it).


# Linking rule and dependencies for the main gap executable
gap$(EXEEXT): $(OBJS) cnf/GAP-LDFLAGS cnf/GAP-LIBS cnf/GAP-OBJS
$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) $(OBJS) $(GAP_LIBS) -o $@

endif


########################################################################
# The "docomp" target regenerates the various src/c_*.c files, and
# replaces the old "etc/docomp" script.
Expand Down Expand Up @@ -458,6 +460,7 @@ clean:
rm -rf obj
rm -f gap$(EXEEXT) gac ffgen
rm -f libgap.la
rm -f libgap.so
rm -f gen/gap_version.c
rm -f doc/wsp.g
rm -f cnf/GAP-{CFLAGS,CPPFLAGS,LDFLAGS,LIBS,OBJS}
Expand Down Expand Up @@ -986,6 +989,12 @@ testbugfix: all
ReadGapRoot( "tst/testbugfix.g" );' | $(TESTGAP) | \
tee `date -u +dev/log/testbugfix2_%Y-%m-%d-%H-%M` )

testlibgap: libgap.la obj/tst/testlibgap/basic.lo
$(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) obj/tst/testlibgap/basic.lo libgap.la -o test-libgap
./test-libgap -A -l $(top_srcdir) -m 32m -q -T --nointeract > basic.out
diff $(top_srcdir)/tst/testlibgap/basic.expect basic.out
.PHONY: testlibgap

coverage:
gcov -o . $(SOURCES)

Expand Down
4 changes: 4 additions & 0 deletions etc/ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ GAPInput

;;

testlibgap)
make testlibgap
;;

*)
if [[ ! -f $SRCDIR/tst/${TEST_SUITE}.g ]]
then
Expand Down
8 changes: 0 additions & 8 deletions lib/streams.gi
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,6 @@ function( stream )
CloseStream(stream);
end );

BindGlobal("LIBGAP_EvalString",
function(string)
local instream, obj;
instream := InputTextString(string);
obj := READ_ALL_COMMANDS(instream, 0, false);
return obj;
end);


#############################################################################
##
Expand Down
62 changes: 62 additions & 0 deletions src/libgap-api.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// LibGAP API - API for using GAP as shared library.

#include "libgap-api.h"
Copy link
Member

Choose a reason for hiding this comment

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

Why libgap-api.h and not just libgap.h? Do you perhaps foresee a future need for a libgap.h with different content (and what would that be)?

Copy link
Member Author

Choose a reason for hiding this comment

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

There used to be a files libgap_internal.h, libgap.h, libgap.c; Also I'd probably want to rename the file at some point anyway, so I didn't really bother coming up with a particularly good name.

Of course gap.h would be the best choice...


#include "bool.h"
#include "opers.h"
#include "calls.h"
#include "gapstate.h"
#include "gvars.h"
#include "lists.h"
#include "streams.h"
#include "stringobj.h"

//
// Setup and initialisation
//
void GAP_Initialize(int argc,
char ** argv,
char ** env,
CallbackFunc markBagsCallback,
CallbackFunc errorCallback)
{
InitializeGap(&argc, argv, env);
Copy link
Member

Choose a reason for hiding this comment

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

Just to better understand: So I guess the long-term plan here is to switch the build system to use -fvisibility=hidden (on platforms where it is available) and then only make the GAP_ functions explicitly public, via an attribute? Say, like so:

#define GAP_EXPORT __attribute__ ((visibility("default")))

GAP_EXPORT void GAP_Initialize( ... )

Of course that would break most kernel extensions right now, but presumably we could convert them all to the new system step by step, too (by adding new dedicated APIs resp. adding GAP_EXPORT qualifiers to more helpers).

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be my preferred goal. Figuring out what the correct API is is the current challenge.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly.

There are two things to concern: How stable do we want the API to be? Having separate function allows us to have a different version for the API than for GAP itself. This is imho a pro for separate functions.

I think using the kernel functions directly would really force us to prefix them all. I am not against this, but it might break a lot of code.

SetExtraMarkFuncBags(markBagsCallback);
STATE(JumpToCatchCallback) = errorCallback;

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

}


// Combines GVarName and ValGVar. For a given string, it returns the value
// of the gvar with name <name>, or NULL if the global variable is not
// defined.
Obj GAP_ValueGlobalVariable(const char * name)
{
UInt gvar = GVarName(name);
// TODO: GVarName should never return 0?
if (gvar != 0) {
return ValGVar(gvar);
}
else {
return NULL;
}
}

//
// Evaluate a string of GAP commands
//
// To see an example of how to use this function
// see tst/testlibgap/basic.c
//
Obj GAP_EvalString(const char * cmd)
{
Obj instream;
Obj res;
Obj viewObjFunc, streamFunc;

streamFunc = GAP_ValueGlobalVariable("InputTextString");
viewObjFunc = GAP_ValueGlobalVariable("ViewObj");

instream = DoOperation1Args(streamFunc, MakeString(cmd));
res = READ_ALL_COMMANDS(instream, False, True, viewObjFunc);
return res;
}
21 changes: 21 additions & 0 deletions src/libgap-api.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// LibGAP API - API for using GAP as shared library.

#ifndef LIBGAP_API_H
#define LIBGAP_API_H

#include "gap.h"

typedef void (*CallbackFunc)(void);

// Initialisation and finalization

void GAP_Initialize(int argc,
char ** argv,
char ** env,
CallbackFunc markBagsCallback,
CallbackFunc errorCallback);

Obj GAP_ValueGlobalVariable(const char * name);
Obj GAP_EvalString(const char * cmd);

#endif
77 changes: 77 additions & 0 deletions tst/testlibgap/basic.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Small program to test libgap linkability and basic working
*/
#include <stdio.h>
#include <unistd.h>
#include <compiled.h>
#include <libgap-api.h>
extern char ** environ;

UInt GAP_List_Length(Obj list)
{
return LEN_LIST(list);
}

Obj GAP_List_AtPosition(Obj list, Int pos)
{
return ELM_LIST(list, pos);
}

UInt GAP_String_Length(Obj string)
{
return GET_LEN_STRING(string);
}

Int GAP_String_GetCString(Obj string, Char * buffer, UInt n)
{
UInt len;

if (IS_STRING(string)) {
if (!IS_STRING_REP(string))
string = CopyToStringRep(string);
len = GET_LEN_STRING(string) + 1;
if (len >= n)
len = n - 1;
// Have to use mempcy because GAP strings can contain
// \0.
memcpy(buffer, CSTR_STRING(string), len);
if (len == n - 1)
buffer[n] = '\0';
return 1;
}
return 0;
}

void test_eval(const char * cmd)
{
Obj res, ires;
Int rc, i;
Char buffer[4096];
printf("gap> %s\n", cmd);
res = GAP_EvalString(cmd);
rc = GAP_List_Length(res);
for (i = 1; i <= rc; i++) {
ires = GAP_List_AtPosition(res, i);
if (GAP_List_AtPosition(ires, 1) == True) {
GAP_String_GetCString(GAP_List_AtPosition(ires, 5), buffer,
sizeof(buffer));
printf("%s\n", buffer);
}
}
}
int main(int argc, char ** argv)
{
printf("# Initializing GAP...\n");
GAP_Initialize(argc, argv, environ, 0L, 0L);
CollectBags(0, 1); // full GC
test_eval("1+2+3;");
test_eval("g:=FreeGroup(2);");
test_eval("a:=g.1;");
test_eval("b:=g.2;");
test_eval("lis:=[a^2, a^2, b*a];");
test_eval("h:=g/lis;");
test_eval("c:=h.1;");
test_eval("Set([1..1000000], i->Order(c));");
printf("# done\n");
return 0;
}
18 changes: 18 additions & 0 deletions tst/testlibgap/basic.expect
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Initializing GAP...
gap> 1+2+3;
6
gap> g:=FreeGroup(2);
<free group on the generators [ f1, f2 ]>
gap> a:=g.1;
f1
gap> b:=g.2;
f2
gap> lis:=[a^2, a^2, b*a];
[ f1^2, f1^2, f2*f1 ]
gap> h:=g/lis;
<fp group on the generators [ f1, f2 ]>
gap> c:=h.1;
f1
gap> Set([1..1000000], i->Order(c));
[ 2 ]
# done