See bug 522118. We will need an upstreamable patch for libffi 3.0.9, once it lands, to get ctypes working on trunk. (I hope to land 3.0.9 sometime in the next week.)

The problem with the unresolved symbol in win32.S on OS/2 appears to be related to the .set pseudo-op. Excluding these lines in win32.S from being build on OS/2 results in a working ffi.a - jsctypes-test passes ok. The only caveat is that configure does not find sed, grep and ar, I have set them before building in the environment.

(In reply to comment #1) > Created an attachment (id=420920) [details] > patch > > The problem with the unresolved symbol in win32.S on OS/2 appears to be related > to the .set pseudo-op. Excluding these lines in win32.S from being build on > OS/2 results in a working ffi.a - jsctypes-test passes ok. > The only caveat is that configure does not find sed, grep and ar, I have set > them before building in the environment. Good catch, I tried everything except removing all the .set pseudo-ops. Did remove some with no change. The set sed, ar and grep can be added to setmozenv.cmd until upstream adds the macro to check for EXEEXT. Did you not have problems with dmalloc?

This is basically just a merge between the harbour dmalloc and the libfii dmalloc. Still missing the spinlock though it seems to work here with the pthread mutex. Also a couple of lines that are commented out which I'm not sure about. Rich, could you take a quick look at this to see if it looks correct?

(In reply to comment #1) > The only caveat is that configure does not find sed, grep and ar, I have set > them before building in the environment. One way to fix this may be to add # set ac_executable_extensions! if ls.exe --version >/dev/null 2>/dev/null; then if test "$ac_executable_extensions" = ""; then AC_MSG_WARN([ac_executable_extensions not set, assuming .exe]) ac_executable_extensions=".exe" export ac_executable_extensions fi fi ]) close to the beginning of configure and configure.ac. Another possibility is to export ac_executable_extensions=".exe" in the parent makefile. Not sure if we also need the PATH and PATH_SEPARATOR fixes

(In reply to comment #2) > Did you not have problems with dmalloc? By +# if (defined(X86_WIN32) && !defined(__OS2__)) || defined(X86_WIN64) in src/closures.c FFI_MMAP_EXEC_WRIT remains undefined on OS2, therefor dmalloc.c won't get included and closures.o contains only a few symbols, probably the same as in 3.0.8, where windows didn't include dmalloc.c as well. That's for now, of course, only a workaround until you manage to get dmalloc.c working for us.

(In reply to comment #3) > Created an attachment (id=420923) [details] > prelimanary patch for dmalloc > Rich, could you take a quick look at this to see if it looks correct? > +#ifdef X86_OS2 I don't recall ever seeing "X86_OS2" before. "__OS2__" is more appropriate. > +#define INCL_DOSMEMMGR Use INCL_DOS to pick up other stuff you'll need. > +/* #include <bsememf.h> */ Not needed. > +/* OS/2 MMAP via DosAllocMem */ > +static void* os2mmap(size_t size) { > + void* ptr; > + if (DosAllocMem(&ptr, size, PAG_COMMIT|PAG_READ|PAG_WRITE) != 0 ) > + return MFAIL; > + return ptr; > +} We need to use himem by including the OBJ_ANY flag. However, on systems that don't have himem, the call will fail with an "invalid parameter" error. So we have to do something like this: if (DosAllocMem(&ptr, size, OBJ_ANY|PAG_COMMIT|PAG_READ|PAG_WRITE) && DosAllocMem(&ptr, size, PAG_COMMIT|PAG_READ|PAG_WRITE)) return MFAIL; > +#if 0 > +/* For direct MMAP, use OBJ_TILE to minimize interference */ > +static void* os2direct_mmap(size_t size) { OBJ_TILE forces segment-aligned low memory. It's the default on all versions except (IIRC) Warp Server Advanced. Since it doesn't do anything we want, this function can go. Rather than using > +#define os2direct_mmap(n) os2mmap(n) it might be simpler to change > +#define DIRECT_MMAP(s) os2direct_mmap(s) to +#define DIRECT_MMAP(s) os2mmap(n) > +static int os2munmap(void* ptr, size_t size) { [...] > + return -1; This should be "return MFAIL;" Also, > + ULONG ulSize = size, ulFlags = 0; The file's coding style defines each variable separately. Other stuff: * malloc_getpagesize & DEFAULT_GRANULARITY should default to 4k & 64k, respectively (which is what we want). You may want to either define these values explicitly or just put in some temporary check to make sure they have these values. * mutexes - I wouldn't rely on "it seems to work here with the pthread mutex". We need native code. Finally... your patch contains CR/LF's. It also has a lot of non-changes (mostly trailing blanks that were removed) - could you delete those chunks?

The patch here is an update for the integration of the newest libffi source. I've managed it to use emxomfar for linking ffi and to create a .lib file such that we can remove the call to emxomfar in the top Makefile when jsctypes is linked. Still the dmalloc part is not activated. Dan, does your wrapper script for MSVC create a "lib"ffi.lib import lib? I've had some problems to convince libtool to add the "lib" lib_prefix to ffi.lib. Usually on OS/2 we would not add a prefix to our library names. We could also change a bit the jsctypes Makefile to read ifeq (WINNT, $(OS_ARCH)) LIB_PREFIX=lib endif SHARED_LIBRARY_LIBS = \ libffi/.libs/$(LIB_PREFIX)ffi.$(LIB_SUFFIX) \ $(NULL) which would allow us to stick to the name ffi.lib And another question is how to recreate properly jsctypes.lib? Make -C $objdir/js/ctypes does not recreate the object files after making the directory clean, only ffi.lib is recreated.

(In reply to comment #7) > Created an attachment (id=430877) [details] > updated patch for the refreshed sources - uses emxomfar > > The patch here is an update for the integration of the newest libffi source. > I've managed it to use emxomfar for linking ffi and to create a .lib file such > that we can remove the call to emxomfar in the top Makefile when jsctypes is > linked. Still the dmalloc part is not activated. I don't think it is a good idea to patch aclocal.m4 as the patch will get overridden the next time automake, or libtool is updated or if someone runs autoreconf.

Several changes in configure in made it necessary to update the patch again. I omitted the changes for aclocal.m4 for comment #8. However, we must not forget the .m4 files, when we want to upstream a patch in the future. Also, this patch uses LIB_PREFIX again in the ctypes/Makefile. Thus, we don't have to change the naming convention in the libffi configure. Certainly something to clarify finally.

Comment on attachment 431481 [details] [diff] [review] patch updated to apply again >diff --git a/js/ctypes/Makefile.in b/js/ctypes/Makefile.in >-ifeq ($(OS_ARCH),OS2) >-# libffi builds an aout lib on OS/2; convert it to an OMF lib. >-libffi/.libs/libffi.$(LIB_SUFFIX): libffi/.libs/libffi.a >- emxomf $< >+ifeq ($(OS_ARCH),WINNT) >+LIB_PREFIX = lib > endif Hum. I had assumed that libffi built a 'libffi.a' on OS/2, just like it does on Windows. And the lines you're removing are consistent with that. Did this change with 3.0.9? Or does your patch to the other files somehow change that? >diff --git a/js/ctypes/libffi/configure b/js/ctypes/libffi/configure >- old_archive_from_new_cmds='emximp -o $output_objdir/$libname.a $output_objdir/$libname.def' >+ if test "$AR" = emxomfar; then >+ old_archive_from_new_cmds='emximp -o $output_objdir/$libname.lib $output_objdir/$libname.def' >+ else >+ old_archive_from_new_cmds='emximp -o $output_objdir/$libname.a $output_objdir/$libname.def' >+ fi Any changes to generated files (aclocal.m4, configure) won't fly :( If you go back to the OS/2 hack we had in js/ctypes/Makefile.in, to generate the OMF lib from the aout, would that work? And not require these changes? >diff --git a/js/ctypes/libffi/configure.ac b/js/ctypes/libffi/configure.ac >- i?86-win32* | i?86-*-cygwin* | i?86-*-mingw*) >+ i?86-win32* | i?86-*-cygwin* | i?86-*-mingw* | i?86-*-os2*) This part will fly. :) Rest looks fine.

(In reply to comment #10) > (From update of attachment 431481 [details] [diff] [review]) > >diff --git a/js/ctypes/Makefile.in b/js/ctypes/Makefile.in > > >-ifeq ($(OS_ARCH),OS2) > >-# libffi builds an aout lib on OS/2; convert it to an OMF lib. > >-libffi/.libs/libffi.$(LIB_SUFFIX): libffi/.libs/libffi.a > >- emxomf $< > >+ifeq ($(OS_ARCH),WINNT) > >+LIB_PREFIX = lib > > endif > > Hum. I had assumed that libffi built a 'libffi.a' on OS/2, just like it does on > Windows. And the lines you're removing are consistent with that. Did this > change with 3.0.9? Or does your patch to the other files somehow change that? Libtool has always built libraries without a lib prefix for 8.3 name compatibility. > > >diff --git a/js/ctypes/libffi/configure b/js/ctypes/libffi/configure > > >- old_archive_from_new_cmds='emximp -o $output_objdir/$libname.a $output_objdir/$libname.def' > >+ if test "$AR" = emxomfar; then > >+ old_archive_from_new_cmds='emximp -o $output_objdir/$libname.lib $output_objdir/$libname.def' > >+ else > >+ old_archive_from_new_cmds='emximp -o $output_objdir/$libname.a $output_objdir/$libname.def' > >+ fi > > Any changes to generated files (aclocal.m4, configure) won't fly :( > > If you go back to the OS/2 hack we had in js/ctypes/Makefile.in, to generate > the OMF lib from the aout, would that work? And not require these changes? The OS/2 hack works fine as long as $LIB_PREFIX is set correctly and -lfii would work for libffi.a, ffi.a, libffi.lib etc. Is there a reason to link to the library name instead of using -lffi?

(In reply to comment #11) > Libtool has always built libraries without a lib prefix for 8.3 name > compatibility. Hmm, I guess libffi_msvc built libffi.a on OS/2, then. OK. > The OS/2 hack works fine as long as $LIB_PREFIX is set correctly and -lfii > would work for libffi.a, ffi.a, libffi.lib etc. > Is there a reason to link to the library name instead of using -lffi? Whatever works, as long as the hack lives in js/ctypes/Makefile.in. :)

(In reply to comment #6) Rich, I tried to refresh the dlmalloc patch according to your recommendations. Most of it works but a few problems arose: > (In reply to comment #3) > > Created an attachment (id=420923) [details] [details] > Rather than using > > +#define os2direct_mmap(n) os2mmap(n) it might be simpler to change > > +#define DIRECT_MMAP(s) os2direct_mmap(s) to > +#define DIRECT_MMAP(s) os2mmap(n) When I do this, later the compiler chokes about: In function 'mmap_alloc': dlmalloc.c:3197: error: 'n' undeclared (first use in this function) > > +static int os2munmap(void* ptr, size_t size) { > [...] > > + return -1; > This should be "return MFAIL;" Also, When I return MFAIL the compiler chokes on "return makes integer from pointer without a cast" > Other stuff: > * malloc_getpagesize & DEFAULT_GRANULARITY should default to 4k & 64k, > respectively (which is what we want). You may want to either define these > values explicitly or just put in some temporary check to make sure they have > these values. > * mutexes - I wouldn't rely on "it seems to work here with the pthread mutex". > We need native code. not yet solved

(In reply to comment #13) > > +#define DIRECT_MMAP(s) os2mmap(n) > > When I do this, later the compiler chokes about: > In function 'mmap_alloc': > dlmalloc.c:3197: error: 'n' undeclared (first use in this function) Sorry, there's a typo. Notice that the argument for DIRECT_MMAP is 's' while the one for os2mmap is 'n'. Making the arguments match will fix the error. > > > +static int os2munmap(void* ptr, size_t size) { > > [...] > > > + return -1; > > This should be "return MFAIL;" Also, > > When I return MFAIL the compiler chokes on "return makes integer from > pointer without a cast" I see what you mean. I guess '-1' is correct. > > * malloc_getpagesize & DEFAULT_GRANULARITY should default to 4k & 64k, > > respectively (which is what we want). You may want to either define > > these values explicitly or just put in some temporary check to make > > sure they have these values. In init_mparams(), there's an "#ifndef WIN32 ... #else" section where the variables that use these defaults are set. Just add an #elif to set them to the appropriate values for OS/2. > > * mutexes - I wouldn't rely on "it seems to work here with the pthread > > mutex". We need native code. > > not yet solved The issue is whether USE_LOCKS is enabled or not. I don't think it is because the compile should have died when it tried to #include <pthread.h>. If it is (or will be) enabled, then we should be able to emulate the Win32 spinlock routines using some of the undocumented stuff Innotek put into gcc.

updated according to comment #6 and comment #14 > The issue is whether USE_LOCKS is enabled or not. I don't think it is because > the compile should have died when it tried to #include <pthread.h>. If it is > (or will be) enabled, then we should be able to emulate the Win32 spinlock > routines using some of the undocumented stuff Innotek put into gcc. USE_LOCKS is defined in closures.c and it gets only defined if FFI_MMAP_EXEC_WRIT is defined first, which is undefined if you're using the main patch. The patch here now defines FFI_MMAP_EXEC_WRIT also for OS/2 but undefines USE_LOCK for now.

(In reply to comment #15) > Created an attachment (id=435494) [details] > updated dlmalloc.patch > Hum, yesterday the updated dlmalloc.patch worked, today I get: E:\mozbuild1\staticlib\components\jsctypes.lib(E:\hg-src\hg\comm-central\mozilla\js\ctypes\libffi\src\closures.c) : error LNK2029: "_os2direct_mmap" : unresolved external Seems due to checkins for bug513778

> Hum, yesterday the updated dlmalloc.patch worked, today I get: > : error LNK2029: "_os2direct_mmap" : unresolved external (In reply to comment #6) > (In reply to comment #3) > Rather than using > > +#define os2direct_mmap(n) os2mmap(n) > it might be simpler to change > > +#define DIRECT_MMAP(s) os2direct_mmap(s) > to > +#define DIRECT_MMAP(s) os2mmap(n) (In reply to comment #16) Changing this back to how Dave's patch was appears to build properly.

Dave, when we use OBJ_ANY shouldn't we link ffi also with the LDFLAG -Zhigh-mem, or would it be enough, when the final jsctypes.lib is linked with that flag (which it is, because of being a moz-tree lib)?

(In reply to comment #18) > Dave, when we use OBJ_ANY shouldn't we link ffi also with the LDFLAG > -Zhigh-mem, or would it be enough, when the final jsctypes.lib is linked > with that flag (which it is, because of being a moz-tree lib)? Walter, -Zhigh-mem instructs gcc to put its heap in high-memory (and make whatever other accommodations are necessary). When you allocate memory using OBJ_ANY, gcc isn't involved. However, gcc does provide a header, os2safe.h, that wraps several API calls that may not be hi-mem safe. Including it may be appropriate if ffi uses any of the wrapped calls. See the very top of toolkit\xre\nsAppRunner.cpp for an example.

In order to keep the changes to js/ctypes/Makefile.in simple and to not change to much in the ffi-build-system I'm passing CFLAGS=-Zomf and AR=emxomfar from the main configure to the configure of ffi. We would build within the mozilla-tree an OMF-library with the extension .a (and not .lib). This would allow in case of upstreaming to build an external ffi.a in a.out format as it was before. However, using this patch alone ffi won't compile due to the missing changes to dlmalloc, I'll post in a second patch.

Rich, thanks for the clarification about the himem issue. I didn't find that ffi uses any of the functions declared in os2safe.h I found however maybe a solution for the mutex/locking issue in dlmalloc. Similar defines are used in the apache rw/_mutex-os2.h header. Using this patch ffi including dlmalloc compiles fine and the ffi tests pass without errors. Note, that you need the patch for the ffi-build-system as well.

Comment on attachment 436957 [details] [diff] [review] patch for the ffi build-system I don't really like libraries having the wrong suffix as I've seen the toolchain get confused. In this case it seems acceptable as the simplest solution.

Comment on attachment 436957 [details] [diff] [review] patch for the ffi build-system Dan, do you agree as well? Note in particular the changes in the Makefile

Comment on attachment 436957 [details] [diff] [review] patch for the ffi build-system Looks good, but this will have to be updated since ctypes just moved into js/src/ctypes. (The configure.in changes will now go in js/src/configure.in; the makefile changes into js/src/Makefile.in.) Also, please add your libffi patches to js/src/ctypes/libffi.patch, and reference this bug# at the top; I'll then push them to the maintainer for upstreaming. Same process for the dlmalloc fixes.

Comment on attachment 436958 [details] [diff] [review] dlmalloc patch including locks Walter, I'd be a lot more comfortable with this if you added explicit initializations for |mparams.page_size| and |mparams.granularity| to init_mparams() (about line 2495). The defaults of 4096 & 65536 are already correct but could get changed by accident. As long as we're using high-memory, default granularity can be any multiple of 4k. However, if low-memory were used, os2munmap() would break if it were anything other than 64k. With that addition, r+...

I slightly reorganized both patches and break it into mozilla bits and ffi bits to ease the upstreaming. Rich, could you please have a look again, whether the initialization of mparams.page_size and mparams.granularity is correct? Dan, the current libffi.patch refers still to the former locations. Shall I update it to find the new locations?

fixed some typos

(In reply to comment #27) > Rich, could you please have a look again, whether the initialization of > mparams.page_size and mparams.granularity is correct? Looks OK.

Dan, everything 's prepared, I have set the chekin-needed keyword. Maybe you want to do it, then you could have a look again at the libffi.patch

OK, I'll take a look today.

In my patchqueue.

Comment on attachment 437955 [details] [diff] [review] ffi bits combined of both patches http://hg.mozilla.org/tracemonkey/rev/cd7dda702ef2

Comment on attachment 437967 [details] [diff] [review] update for js/src/ctypes/libffi.patch http://hg.mozilla.org/tracemonkey/rev/89dfba5eaf2c

Comment on attachment 438040 [details] [diff] [review] mozilla bits of both patches - little bitrot http://hg.mozilla.org/tracemonkey/rev/639ee520ad1a

Pushed the libffi bits upstream; I believe they've been applied.

(In reply to comment #38) > Pushed the libffi bits upstream; I believe they've been applied. Yes, they were already checked in, thank you for supporting us.

This builds and runs fine, tests are all ok and libffi can be built standalone as a dll with AR.EXE (aout format) -> verfied