522118 - How to go on with ctypes on OS/2?

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a1pre) Gecko/20091013 Minefield/3.7a1pre
Build Identifier: 

With the current configure script of js/ctypes/configure the OS/2 build bails out when configure is testing for a valid grep. Obviously, as said in the newsgroup this appears to be a problem with the path separators. Nevertheless, we have problems later in the build because libffi is not yet ported to OS/2.
Dave, Rich you said in the newsgroup that you were (partly) successful in building the libffi. Do you see the light at the end or should we disable for now building the ctypes until a proper fix can be committed?

Reproducible: Always

Comment 1

[Dave Yeo]

The problem is ash. Download pdksh, http://hobbes.nmsu.edu/download/pub/os2/util/shell/pdksh-5.2.14-bin-2.zip, extract sh.exe and add it to moztools and edit your setmozenv.cmd replacing all occurrences of ash with sh and try running the configure script.

Comment 2

[Walter Meinl]

pdksh gave me the same result, it didn't find me a valid grep. SET grep=E:\moztools and set PATH_SEPARATOR=";" worked even with ash.exe. Then the build failed as you described in the newsgroup at sysv.S and I did the trick copying over win32.S to sysv.S and adding the underscore to get "globl _ffi_prep_args". Then the build went on, but created only .a archives not .lib files. It finally bailed out after building mozjs.dll and the xpcom libraries when returning back into js/ctypes "no rule to make target libffi/.libs/ffi.lib".

Comment 3

[Dave Yeo]

Interesting, I thought the ash vs sh was the difference in our environments.
For now you can run emxomf on ffi.a to create a ffi.lib. I'd guess that eventually libffi will use the Mozilla build system.
One solution would be to change the requirement for libffi/.libs/ffi.lib to -Llibfii/.libs -lffi, as then the aout to omf conversion would be automatic (even just changing ffi.lib to ffi.a would also work)

Comment 4

[[e-mail]]

(In reply to comment #3)
> Interesting, I thought the ash vs sh was the difference in our environments.
> For now you can run emxomf on ffi.a to create a ffi.lib. I'd guess that
> eventually libffi will use the Mozilla build system.

We want to keep libffi as close to upstream as possible, so it won't use our build system. If we can patch it and push upstream, that would be ideal. If you can get something working and reasonable, I can certainly help with the upstreaming bit.

Comment 5

[Dave Yeo]

Attachment: link to ffi.a instead of ffi.lib

As the plan is to keep libffi using libtool to build the static lib and libtool builds aout libs on OS/2 (hence the .a suffix) this small patch will link against ffi.a and allow the build system to automatically do the aout to OMF conversion. This will also still be needed once OS/2 support is merged into upstream.
Dan, asking you for review as you seem to be involved with integrating libfii into Mozilla

Comment 6

[Dave Yeo]

Comment on attachment 406893 [details] [diff] [review]
link to ffi.a instead of ffi.lib

Sorry, I thought this patch was correct but  it isn't. I'll have another after more testing

Comment 7

[Walter Meinl]

Attachment: workaround 1 disable libffi in configure

If we don't find a proper solution (comments later) to fix the libtool problems we should do sth like this.

Comment 8

[Walter Meinl]

Attachment: workaround 2, works only when cflags=-Zomf and AR=emxomfar are set

This simple patch tells configure to use "lib" and not "a" as extension for an archive. However, the only way I got it really working was to set CFLAGS=-Zomf and AR=emxomfar _before_ I run make -f client.mk
I tried the last 2 weeks to get something going that would set the right flags/archiver, but it ended always with either emxomfar: record too long or emxomfar: unexpected end of file.
One of the most promising attempts was to let the mozilla configure.in pass these these env variables over when libffi configure is invoked:
(close to the end of configure.in is the section to invoke libffi/configure)
    if test "$OS_ARCH" = "OS2"; then
      ac_configure_args="CFLAGS=-Zomf AR=emxomfar $ac_configure_args"
    fi
While this worked for the CFLAGS it didn't work for AR.
Further attempts were to set these variables in every os2* section in configure and all lt* files that have os* parts.
All these attempts ended with either emxomfar (some-o-file) record too long or unexpected end of file --stop.

Comment 9

[Walter Meinl]

Another question is whether the usage of the win32.S assembler file is correct for OS/2. Dave, do you think it is?

Comment 10

[Peter Weilbacher]

Good question. How can we test, if the FFI stuff works once a build is finished? Or the other way around: do we notice that it is missing when using the workaround from attachment 409483 [details] [diff] [review]?

Comment 11

[[e-mail]]

ctypes has unit tests you can run; |cd $OBJDIR/js/ctypes/tests && make xpcshell-tests" will run them. Having those pass is a pretty good indication that things are working.

Actually, come to think of it, the test only knows about XP_UNIX, XP_WIN, and XP_MACOSX. So the test will bail early. We should change that, I'll post a patch in a bit.

Anyways, attachment 409483 [details] [diff] [review] just disables ctypes entirely, so there won't be any tests to run. With attachment 409485 [details] [diff] [review] you should get an answer, though.

Comment 12

[[e-mail]]

Attachment: enable ctypes unit tests on all platforms (checked in trunk & branch)

Per previous comment, this should make the tests run on OS2. (Yay preprocessor!)

Comment 13

[Dave Yeo]

(in reply to comment #8)
You also might need a RANLIB=echo. The problem is that the Mozilla build is
looking for fii.lib and builds a static archive with fii.lib as a member. This
confuses the automatic OMF conversion. I'll post a preliminary patch.
(in reply to comment #9)
I think that the win32.S should be close to what we need but I'm no expert.
We'll need to run the unit tests.

Comment 14

[Dave Yeo]

Attachment: Add a rule to convert ffi.a to ffi.lib on OS/2

This patch gets around the no rule to make ffi.lib error. This will be needed even when we get OS/2 support upstream as libtool builds aout libs by default and even with -Zomf still names them with the .a suffix.
I think this patch can be improved by replacing some of it with variables so not asking for review yet.

Comment 15

[Walter Meinl]

(In reply to comment #12)
> Created an attachment (id=409553) [details]
> enable ctypes unit tests on all platforms
> 
Dan, your patch works fine, however when I tried to run the test I got an error about the library not being found. The problem is the designation of the test dll, which has to take a SHORT_LIBNAME on OS/2 (dlls must not exceed 8+3 names). I'd suggest to add a line to Makefile.in like SHORT_LIBNAME = jscttest
Having done this locally in my tree the dll got loaded successfully.

Comment 16

[Walter Meinl]

While the test initially runs fine (using SHORTLIB_NAME as stated in comment#15) it fails later
TEST-PASS | E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | [
run_int8_tests : 140] 10 == 10
TEST-UNEXPECTED-FAIL | E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | 252 == 255 - See following stack:
JS frame :: E:/usr/src/hg/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 197
JS frame :: E:/usr/src/hg/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 227
JS frame :: E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js ::run_int8_tests :: line 145
JS frame :: E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js ::run_test :: line 71
JS frame :: E:/usr/src/hg/mozilla-central/testing/xpcshell/head.js :: _execute_test :: line 125
JS frame :: -e :: <TOP_LEVEL> :: line 1

Comment 17

[Walter Meinl]

I removed in test_jsctypes.js the functions for int8 to int64 to test if other tests fail as well, but float, double, string and mixed pass. So we have to find out why the functions in comment#16 fail.
However, Dan another need of OS/2 for your test came up: syslib.close() is undefined. We would need to add
   var syslib = ctypes.open("libm.dylib");
 #elifdef XP_UNIX
   var syslib = ctypes.open("libm.so");
+#elifdef XP_OS2
+  var syslib = ctypes.open("libc063.dll");
 #else
   // nothing run this test
 #endif
close to the end of mozilla/js/ctypes/tests/unit/test_jsctypes.js.in.
If you like I could incorporate the two changes for OS/2 into your patch attachment (id=409553)

Comment 18

[Walter Meinl]

Strange, when I run the test with python244 I get
TEST-PASS | E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | [
run_int8_tests : 140] 10 == 10
TEST-UNEXPECTED-FAIL | E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | 60 == 255 - See following stack:
JS frame :: E:/usr/src/hg/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 197
the rest of the error message is the same
but note with python244 it errors about: 60 == 255 
with python26 (comment#16) it errors about: 252 == 255
with virtually the only difference is the python version.

Comment 19

[Dave Yeo]

Walter could you post your updated patch? I seem to have screwed up somewhere with the test erroring out right away with
failed to get nsJSRuntimeService!

Comment 20

[Walter Meinl]

Attachment: added a few os/2 bits to dwitte's ctypes unit test patch

Comment 21

[Dave Yeo]

I couldn't get Python 2.44 to run the test suite. With 2.6 I get
156=255
playing around with different numbers the left hand number was always the same
I've also got a 1.7 MB patch adding os2.S to the build system (right now os2.S is just the patched win32.S). It is so big as I updated configure.ac and makefile.am and ran automake and autoconf on them. Should I post it?

Comment 22

[Dave Yeo]

Attachment: A better rule to convert ffi.a to ffi.lib

Comment 23

[Walter Meinl]

(In reply to comment #21)
> I couldn't get Python 2.44 to run the test suite.
Edit config/autoconf.mk to read the path of python244 and start in a new window setmozenv pointing to python244. It could be that you get some more output than with python2.6, because there is a workaround in the pythonscript invoking the xpcshell tests, as the subprocess module doesn't work properly in 2.4.4
>  With 2.6 I get 156=255
Interesting
> playing around with different numbers the left hand number was always the same
> I've also got a 1.7 MB patch adding os2.S to the build system (right now os2.S
> is just the patched win32.S). It is so big as I updated configure.ac and
> makefile.am and ran automake and autoconf on them. Should I post it?
Why not.

Comment 24

[Dave Yeo]

Attachment: Build system changes for OS/2 and os2.S. Needs more work.

This is a first take on configure.am and makefile.am changes to enable OS/2 including a first shot at os2.S on libfii. Also includes the results of running automake and autoconf on libfii.
File is gzipped due to bandwidth considerations.

Comment 25

[Dave Yeo]

With attachment 409654 [details] and attachment 409650 [details] [diff] [review] and using Python 2.6 the build goes smooth and the unit tests now pass. (seems we needed to build win32.S (now os2.S) and sysv.S
Walter can you confirm?

Comment 26

[[e-mail]]

Comment on attachment 409654 [details]
Build system changes for OS/2 and os2.S. Needs more work.

Changing mimetype. (For some reason not adding a comment doesn't let me do it, but maybe it won't work now either.)

Comment 27

[[e-mail]]

Ah. It needs to not be a patch to be a non-text/plain mimetype.

Comment 28

[[e-mail]]

Comment on attachment 409654 [details]
Build system changes for OS/2 and os2.S. Needs more work.

Doesn't look like this patch includes os2.S. I'm interested in the differences, though - can you attach a diff between it and win32.S? I assume that's the important bit in what got the tests passing for you.

Comment 29

[Dave Yeo]

Actually I thought this would just be preliminarily to hacking on os2.S, the only change between the 2 files was
--- win32.S     Sat Sep 26 20:07:58 2009
+++ os2.S       Mon Nov  2 15:33:12 2009
@@ -33,7 +33,7 @@

 .text

-.globl ffi_prep_args
+.globl _ffi_prep_args

         # This assumes we are using gas.
         .balign 16

I can't really see what is different between building with os2.S vs win32.S, perhaps some define or using an OS/2 autoconf. I'll try some comparisons.
I'll see if we can't just use win32.S with an ifdef for the above.
Also need to check about enabling stdcall

Comment 30

[[e-mail]]

Comment on attachment 409553 [details] [diff] [review]
enable ctypes unit tests on all platforms (checked in trunk & branch)

http://hg.mozilla.org/mozilla-central/rev/c6e8c9620f41

Comment 31

[Walter Meinl]

(In reply to comment #29)

> Also need to check about enabling stdcall
Dave, there are quite a few bits in libffi/src/x86/ffi.c and ffitarget.h that are if(n)defed X86_WIN32 which also use stdcall, maybe we need a bit of that, do we? What about stdcall and OS/2 at all?

Comment 32

[Dave Yeo]

Seems that OS/2 does support stdcall, referenced in the Alp (OS/2 assembler) documentation, the libc release notes and it looks like Andrew fixed a GCC bug related to stdcall (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12344). As well OpenWatcom uses it. Looks like we can just add OS/2 to the ifdefs.
I never had luck using win32.S due to libtool weirdness and the win32.S from 3.0.9rc3 has had a major rewriting to support the eh_frame. Since OS/2 does not support eh_frame I think the best move is to use the os2.S created by copying win32.S from 3.0.8. Wonder how to do the license header since we haven't done any changes to speak of yet? os2.S copied from win32.S etc or os2.S based on win32.S etc
I'll try to get a patch together against 3.0.9rc3 tonight or tomorrow.

Comment 33

[[e-mail]]

(In reply to comment #32)
> I'll try to get a patch together against 3.0.9rc3 tonight or tomorrow.

Neat. Have you run this by the libffi mailing list, to see what approach they suggest and whether they'd take the patch?

Comment 34

[Walter Meinl]

Attachment: Dave's OS/2 build system patch regenerated with linux autoconf/make

Dave, I took your configure.ac and Makefile.am changes and recreated them with autoconf-2.63 and automake-1.10.1 on linux, which shortened enormously the whole thing (probably your patch was such huge due to DOS line endings in the regenerated configure).
This version takes win32.S (we can easily switch back to os2.S). I added some X86_OS2 defines next to the win32 in ffi.c and ffitarget.h.
However the test still fails now with 118 == 255. Not sure, what's different in your setup that the test passes.

Comment 35

[Walter Meinl]

Attachment: os/2 bits needed to make the ctypes unit test work

comment#15 and comment#17

Comment 36

[Dave Yeo]

(in reply to comment #33)
I haven't posted anything to the libfii list yet. I'd rather show up with a patch in hand.
I was hoping to just use win32.S with a few ifdefs as it would be more likely to be accepted. Unluckily trying to link with the 3.0.9rc3 win32.S hasn't worked out due to the ___eh_frame__ section.
Anyways once we figure out why I passed and Walter failed I'll have to post there.
(in reply to comment #34)
Thanks for recreating them with the correct autoconf and automake versions. I think my patch was bloated due to using different versions.
I'm rebuilding to test your version then we'll have to try to figure out the differences.
Did you ever test with my patch (after adding os2.S)

Comment 37

[Walter Meinl]

(In reply to comment #36)

> Did you ever test with my patch (after adding os2.S)
Yes, but it wasn't successful either (again a different number IIRC 60 == 255)

Comment 38

[Dave Yeo]

Weird, perhaps it was just a coincidence that my tests passed, eg randomly 255==255.
I haven't been able to get your version of the patch to link here, seems not happy with the underline prefix with this error

I:\comm-central\.mozilla-trunk\obj-fb\staticlib\components\jsctypes.lib(win32.obj) : error LNK2029: "ffi_prep_args" : unresolved external

Still testing.

Comment 39

[Dave Yeo]

> I:\comm-central\.mozilla-trunk\obj-fb\js\ctypes\tests >make xpcshell-tests
i:/python26/python.exe -u I:/comm-central/.mozilla-trunk/config/pythonpath.py \
          -II:/comm-central/.mozilla-trunk/build \
          I:/comm-central/.mozilla-trunk/testing/xpcshell/runxpcshelltests.py \
          --symbols-path=../../../dist/crashreporter-symbols \
          ../../../dist/bin/xpcshell \
          ../../../_tests/xpcshell/jsctypes-test/unit
TEST-PASS | I:/comm-central/.mozilla-trunk/obj-fb/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | test passed
INFO | Result summary:
INFO | Passed: 1
INFO | Failed: 0

Comment 40

[Walter Meinl]

(In reply to comment #38)

> I haven't been able to get your version of the patch to link here, seems not
> happy with the underline prefix with this error
> 
> I:\comm-central\.mozilla-trunk\obj-fb\staticlib\components\jsctypes.lib(win32.obj)
> : error LNK2029: "ffi_prep_args" : unresolved external
hm, I had the same problem when I put #ifdef X86_OS2 into win32.S, then I tried a bit and came to #ifdef _OS2 which compiled successfully (however, I made only a clean of libffi and toolkit/library and recompiled both, not the whole tree).
When I don't do an ifdef _OS2 and add the underscore unconditionally linking works. It appears that the ifdef Whatever_OS2 macro doesn't get recognized or used properly.

(In reply to comment #39)
maybe I have to recompile the tree with python26 (I don't usually do it, though it works, cause with python26 I have trouble with mercurial queues and until now it was fine to compile with 244 and then edit autoconf.mk to use python26 before running the xpcshell tests with python26).

Comment 41

[Walter Meinl]

Attachment: Dave's patch regenerated with linux autoconf/make

> I haven't been able to get your version of the patch to link here, seems not
> happy with the underline prefix with this error
It appears #ifdef __OS2__ in the win32.S file works, could build from scratch.

Comment 42

[[e-mail]]

(In reply to comment #38)
> Weird, perhaps it was just a coincidence that my tests passed, eg randomly
> 255==255.

It's possible, though unlikely, that the xpcshell harness is tricking you. (I've run into this sort of thing before.) You can double-check by running the test in non-summarizing mode:

make SOLO_FILE=test_jsctypes.js check-interactive

followed by _execute_test() and quit(). That'll print all the test output so you can be sure it passed...

Comment 43

[Dave Yeo]

(in reply to comment #42)
It passes fine ran this way as well.
The question is why does it not pass for Walter.

Comment 44

[[e-mail]]

There are a few likely possibilities for failing tests:

1) gcc defaults to a non-cdecl calling convention on OS/2, and thus compiles the test library with a different convention than expected. (From what I've found, gcc's after 3.2 default to cdecl. So this is unlikely.)
2) win32.S has a windows-specific implementation of cdecl, and OS/2's is slightly different.
3) -Zomf alters gcc's default behavior with regard to calling convention.

My first question would be, what gcc and toolchain versions, and build flags, are you both using? This may be the reason Walter's test passes while Dave's fails.

The fact that the int8 tests passed for int8_t values, and failed with uint8_t, is curious. OS/2's ABI for unsigned integers may be significantly different, but that seems pretty strange to me. We need more information here. What happens with the int16, int32, and int64 tests? Do they also fail at the same point, fail earlier, or pass?

In and of itself, that Walter is getting seemingly random values in his failures isn't all that interesting, it just means we got the calling convention wrong.

As a footnote, I'd also note that OS/2 has an additional calling convention. From the syscall section of http://en.wikipedia.org/wiki/X86_calling_conventions: "This is similar to cdecl in that arguments are pushed right to left. EAX, ECX, and EDX are not preserved. The size of the parameter list in doublewords is passed in AL. Syscall is the standard calling convention for 32 bit OS/2 API." Thus to support OS/2 API's, we'll need an implementation of syscall. (We can get to that later, though, one thing at a time. I'm also not sure if OS/2 even has stdcall.)

Comment 45

[[e-mail]]

Attachment: test unsigned int ABI

This adds explicit C functions for testing unsigned ints, rather than reusing the signed int versions. If there's a difference in ABI between signed and unsigned ints, this should pass.

Dave, Walter, could you test this patch?

Comment 46

[Dave Yeo]

(replying to comment #44)
> 1) gcc defaults to a non-cdecl calling convention on OS/2, and thus compiles
> the test library with a different convention than expected. (From what I've
> found, gcc's after 3.2 default to cdecl. So this is unlikely.)

This is a possibility, in which case it should be syscall. According to the system docs OS/2 supports C (cdecl), stdcall, syscall, and optlink. Optlink is a Visual Age compiler thing which I don't think it is worth worrying about.

> 2) win32.S has a windows-specific implementation of cdecl, and OS/2's is
> slightly different.

This is the other possibility. OS/2 started out using the MS tool chain but they may well of diverged with the 32 bit version. This was one reason to start with win32.S
 
> 3) -Zomf alters gcc's default behavior with regard to calling convention.

I'm pretty sure this is not the case. The -Zomf (or running emxomf) just converts the object type.

> My first question would be, what gcc and toolchain versions, and build 
> flags,
> are you both using? This may be the reason Walter's test passes while Dave's
> fails.

I'm using binutils-2.19.1-os2-20090427.zip and gcc-4.4.1-os2-20090810.zip. I've also tested building libffi with GCC 3.3.5 and the default toolchain and GCC 3.4.6. Tests passed with these as well.
CFLAGS = -Wall -g -fexceptions -02 -MT -MD -MP -MF
ASFLAGS = -g -02 -MT -MD -MP -MF

> The fact that the int8 tests passed for int8_t values, and failed with 
> uint8_t,
> is curious. OS/2's ABI for unsigned integers may be significantly different,
> but that seems pretty strange to me. We need more information here. What
> happens with the int16, int32, and int64 tests? Do they also fail at the 
> same point, fail earlier, or pass?

As far as I know our int* and uint* are simply copied from FreeBSD 5.1 along with the rest of the libc headers. The OS/2 headers (toolkit) are too old for the int types. They do have most of the same types as Windows.
The int16, int32 and int64 tests pass here.

> As a footnote, I'd also note that OS/2 has an additional calling convention.
> From the syscall section of
> http://en.wikipedia.org/wiki/X86_calling_conventions: "This is similar to 
> cdecl
> in that arguments are pushed right to left. EAX, ECX, and EDX are not
> preserved. The size of the parameter list in doublewords is passed in AL.
> Syscall is the standard calling convention for 32 bit OS/2 API." Thus to
> support OS/2 API's, we'll need an implementation of syscall. (We can get to
> that later, though, one thing at a time. I'm also not sure if OS/2 even has
> stdcall.)

Yes I've had that wikipedia article open for days :) Modifying the cdecl to support syscall shouldn't be too hard once we get consistent results for cdecl.
If nothing else stdcall is used by OpenWatcom which quite a few people use.

Comment 47

[[e-mail]]

(In reply to comment #46)
> As far as I know our int* and uint* are simply copied from FreeBSD 5.1 along
> with the rest of the libc headers. The OS/2 headers (toolkit) are too old for
> the int types. They do have most of the same types as Windows.
> The int16, int32 and int64 tests pass here.

Hmm. A simple way to check if there's any ABI difference between int8_t and uint8_t would be to apply attachment 411581 [details] [diff] [review] and compare the assembly generated for test_ui8_ui8 and test_i8_i8. They should be identical.

Comment 48

[Rich Walsh]

(In reply to comment #44)

> 1) gcc defaults to a non-cdecl calling convention on OS/2
> 2) win32.S has a windows-specific implementation of cdecl, and OS/2's is
> slightly different.

My impression is that the people who ported gcc 3.x tried to be as faithful to a BSD-style implementation as possible, making #1 unlikely.

> As a footnote, I'd also note that OS/2 has an additional calling convention.
> From the syscall section of [wikipedia]: "This is similar to cdecl
> in that arguments are pushed right to left. EAX, ECX, and EDX are not
> preserved. The size of the parameter list in doublewords is passed in AL.
> Syscall is the standard calling convention for 32 bit OS/2 API."

The term "syscall" is only used by the Watcom compiler.  Everywhere else, it's identified as "_System" (gcc & the IBM compilers which are still widely used).  Although it's documented that the size of the parmlist is passed in AL, I find no evidence that this was ever implemented.  It definitely is not referenced by the API.

> I'm also not sure if OS/2 even has stdcall.

Watcom & gcc may support it, but other than the OS/2 port of Wine, I can't imagine why anyone writing for OS/2 would choose to use it.  OTOH, a great deal of the existing OS/2 codebase uses the IBM compilers' "optlink" convention.  (A lot of our older plugins used it, which may be why they don't work anymore...)

Comment 49

[Dave Yeo]

(in reply to comment #47)
The test passes fine with attachment 411581 [details] [diff] [review] with test_jsctypes.js.log being identical.
Comparing the assembler output will have to wait for this evening.

Comment 50

[Walter Meinl]

(in reply to comment #47)
The test brakes with or without attachment 411581 [details] [diff] [review] when testing for uint8 and uint16, while uint32 and uint64 pass ok. Today it breaks for uint8 with 188==255, yesterday it was 92==255.
When I play around with the argument of test_ui8_ui8(0xff) and exchange it to whatever value > 0 < 0xff it constantly "expects" 188 (yesterday 92, other days other numbers see above). When I use a "-" sign in front, e.g. -1 or -0xff it calculates the absolute value correctly, i.e. -1 or -255, but (of course) complains about a type error TypeError: expected type uint8, got -255. All the latest compilations were done with attachment (id=410896), maybe I've overlooked something there. Another explanation could be that I'm running on ECS 2.0rc7.

Comment 51

[[e-mail]]

(In reply to comment #50)
> When I play around with the argument of test_ui8_ui8(0xff) and exchange it to
> whatever value > 0 < 0xff it constantly "expects" 188

Ok, so that means the function is reading a different location from where the arg was pushed. We need to compare the assembly the compiler generates for test_i8_i8, test_i16_i16, and test_i32_i32 to what win32.S is doing. Can you and Dave attach the assembly for those three functions?

> All the
> latest compilations were done with attachment (id=410896) [details], maybe I've
> overlooked something there.

That part looks fine to me.

Comment 52

[[e-mail]]

Comment on attachment 410896 [details] [diff] [review]
os/2 bits needed to make the ctypes unit test work

>+#elifdef XP_OS2
>+  var syslib = ctypes.open("libc063.dll");
> #else
>   // nothing run this test
> #endif
>   syslib.close();
>   return true;

The #else branch will throw because syslib isn't defined (and before your patch, as well). Let's make it explicit with something like do_throw("please add a system library for this test").

Comment 53

[Dave Yeo]

Attachment: partial assembler output from jsctypes-test.cpp

Here's some of the output from adding -S to the CXXFLAGS and compiling jsctypes-test.cpp.

Comment 54

[Walter Meinl]

Attachment: assembly listing g++ -Wa,-ahl=jsctypes-test.s

Not sure, if I did it right, see the compile-flags I added when recompiling jsctypes-test.cpp (simply adding -s to the CXXFLAGS produced an empty file)

Comment 55

[Walter Meinl]

Attachment: make ctypes unittests work on OS/2 comment 52 addressed (checked in on trunk & branch)

comment #52 addressed ready for checkin

Comment 56

[Dave Yeo]

(in reply to comment #54)
Interesting output. I added the -S (note capital S) to the end of the OS_CXXFLAGS in %OBJDIR%\config\autoconf.mk so preserving the compile flags.
Your attachment (id412066) has the same assembly code as mine.
(in reply to comment #23)
Still haven't been able to run the testsuite with 2.44 with the error being
failed to get nsJSRuntimeService!
I started out with 2.44 and edited autoconf.mk to change versions. Since I've been using 2.6 except my last build which I used 2.44. Running the tests with 2.6 still passes.
I'll send you my ffi.a privately. Then you can make clean in js/ctypes and js/ctypes/test and recompile and test.

Comment 57

[Walter Meinl]

(In reply to comment #56)
> (in reply to comment #54)
> I'll send you my ffi.a privately. Then you can make clean in js/ctypes and
> js/ctypes/test and recompile and test.
Bingo, the test passes with your ffi.a, at least with python2.6. With python2.44 it still fails.
Your ffi.a is smaller than that I compiled. Using nm I found that yours doesn't contain raw_api.o and java_raw_api.o, while mine does (though I found in my fficonfig.h #define FFI_NO_RAW_API 1 and configure was invoked with --disable-raw-api).
On a side-note, I had to recompile the whole tree, because "Make -C js/ctypes" does not recompile jsctypes.lib (after a clean the lib and all object files are gone and won't be made again).

Comment 58

[Walter Meinl]

Dave, I compiled the bits on linux and the libffi.a doesn't contain raw_api.o and java_raw_api.o. I guess, since I used my patch for compilation, that there is still a bit wrong and the errors might be due to the raw_api inclusion. Did you ever try my patch and got an error-free ffi with it?

Comment 59

[Walter Meinl]

Attachment: updated the regenerated patch for a little bit-rot

Dave, ignore comment #58. I _should_ have run another test before posting that comment. After cleaning my source tree and setting up my build tools again, the patch works now also here (still I don't know what made it fail).
Output with python-2.4.4:
TEST-PASS | (xpcshell/head.js) | 73 (+ 0) check(s) passed
TEST-PASS | E:/mozbuild1/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | test passed
INFO | Result summary:
INFO | Passed: 1
INFO | Failed: 0
make.exe: Leaving directory `E:/mozbuild1/js/ctypes/tests'

Rich, did you ever try to run the test in order to get another confirmation that we can go for review of the patch?

Comment 60

[Rich Walsh]

(In reply to comment #59)
> Rich, did you ever try to run the test in order to get another confirmation
> that we can go for review of the patch?

Sorry, but I typically don't build tests, and in fact, until a couple of days ago I had never built ctypes because this bug seemed to be an unresolved work-in-progress.  Using source from 2009-12-20 & the patches as they were on that date (plus various additions to setmozenv.cmd), ctypes finally built OK - but that might not be saying anything meaningful.  After Christmas, I can build with tests enabled & try it.

Comment 61

[Rich Walsh]

(In reply to comment #60)
> (In reply to comment #59)
>>
>> Rich, did you ever try to run the test in order to get another confirmation
>> that we can go for review of the patch?
> 
> After Christmas, I can build with tests enabled & try it.

I assume that this is what you were looking for:

TEST-PASS | M:/browser441/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | test passed

Comment 62

[Walter Meinl]

Comment on attachment 419114 [details] [diff] [review]
updated the regenerated patch for a little bit-rot

(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #59)

> I assume that this is what you were looking for:
> 
> TEST-PASS | M:/browser441/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js |
> test passed

Yes, thanks, should have asked you earlier.

Dan, our patch is working now and does not change too much. How is the correct procedure for getting this into a) the tree (and mozilla-1.9.2) and b) upstream?

Comment 63

[[e-mail]]

Comment on attachment 419114 [details] [diff] [review]
updated the regenerated patch for a little bit-rot

>diff --git a/js/ctypes/Makefile.in b/js/ctypes/Makefile.in

>+ifeq ($(OS_ARCH),OS2)
>+libffi/.libs/ffi.$(LIB_SUFFIX): libffi/.libs/$(LIB_PREFIX)ffi.a
>+	emxomf $<
>+endif
>+
>+

This looks fine (superfluous newline, though).

>diff --git a/js/ctypes/Function.cpp b/js/ctypes/Function.cpp

>-#if defined(_WIN32) && !defined(_WIN64)
>+#if defined(_WIN32) && !defined(_WIN64) || defined(_OS2)
>   case ABI_stdcall_abi:
>     aResult = FFI_STDCALL;
>     return true;

Can you put parens around the _WIN32 && !_WIN64 for clarity?

>diff --git a/js/ctypes/libffi/Makefile.am b/js/ctypes/libffi/Makefile.am

>+if X86_OS2
>+nodist_libffi_la_SOURCES += src/x86/ffi.c src/x86/win32.S
>+endif 

Hmm. X86_OS2 seems identical to X86_WIN32. Can we just merge them? That would make the patch much smaller. And then...

>diff --git a/js/ctypes/libffi/configure.ac b/js/ctypes/libffi/configure.ac

>+  i386-*-os2*)
>+	TARGET=X86_OS2; TARGETDIR=x86
>+	;;

... have TARGET=X86_WIN32 here. That would save having (X86_WIN32 | X86_OS2) in the sources everywhere, which is prone to human error.

>diff --git a/js/ctypes/libffi/src/x86/win32.S b/js/ctypes/libffi/src/x86/win32.S

>+#ifdef __OS2__
>+.globl _ffi_prep_args
>+#else
> .globl ffi_prep_args
>- 
>+#endif

Hm. Was this necessary to build? I don't see ffi_prep_args referenced anywhere in this file. What happens without this change (or even better, if you unconditionally remove this whole chunk)?

We can get your patch into mozilla-central first, then we can push it upstream to the maintainer of libffi. He's about to do a release of libffi 3.0.9 today or tomorrow, I think, but we can carry the patch in m-c until the next release.

Comment 64

[Dave Yeo]

(In reply to comment #63)

>>diff --git a/js/ctypes/libffi/Makefile.am b/js/ctypes/libffi/Makefile.am
>
>>+if X86_OS2
>>+nodist_libffi_la_SOURCES += src/x86/ffi.c src/x86/win32.S
>>+endif 
>
>Hmm. X86_OS2 seems identical to X86_WIN32. Can we just merge them? That would
>make the patch much smaller. And then...

The problem is that win32.S seems to have been mostly rewritten, mostly it seems to support the eh_frame which is not supported by our aout object format.

[...]

>>diff --git a/js/ctypes/libffi/src/x86/win32.S b/js/ctypes/libffi/src/x86/win32.S
>
>>+#ifdef __OS2__
>>+.globl _ffi_prep_args
>>+#else
>> .globl ffi_prep_args
>>- 
>>+#endif
>
>Hm. Was this necessary to build? I don't see ffi_prep_args referenced anywhere
>in this file. What happens without this change (or even better, if you
unconditionally remove this whole chunk)?

Without the leading underscore we get an unresolved external error. Will try removing the whole chunk.

>We can get your patch into mozilla-central first, then we can push it >upstream
>to the maintainer of libffi. He's about to do a release of libffi 3.0.9 today
>or tomorrow, I think, but we can carry the patch in m-c until the next >release.

The patch against 3.0.9 will be different from this patch so ideally would be this patch in mozilla-central and a patch against 3.0.9 upstream.
I'm currently trying to get git working here to get the latest upstream source

Comment 65

[Dave Yeo]

Attachment: Proposed patch for upstream

This builds fine with 3.0.9rc9 downloaded from http://github.com/atgreen/libffi.
Attempts to go totally the win32 route failed due to missing mmap in src/dmalloc.c and I couldn't see what was different.
Untested due to local build problems. Walter would it be possible for you to test this in the Mozilla tree?

Comment 66

[[e-mail]]

How did you get around the dlmalloc.c mmap problem? (Does it no longer get built with the new X86_OS2?) You may be able to disable the use of malloc on OS2... grep for 'FFI_MMAP_EXEC_WRIT' in libffi/configure.ac; apparently it's disabled on some platforms, so you may just be able to add OS2 to that list.

A quick googling also turned up this: http://www.mail-archive.com/[e-mail]/msg21943.html ... it seems dlmalloc needs to use different mmap API's on OS2. Someone apparently did the port, but I'm not sure where it is.

To respond to your earlier comment, we're going to keep libffi 3.0.8 on the mozilla 1.9.2 branch, and update to 3.0.9 on mozilla-central. So we will probably need two different patches for the two; a small, minimal patch for 1.9.2 branch, and then something we can upstream for mozilla-central. (Unless you don't care about 1.9.2, of course.)

Comment 67

[Dave Yeo]

With the new X86_Os2 dmalloc.c does not get built and the right defines happen so it is not missed. Thanks for the hint about FFI_MMAP_EXEC_WRIT, that's what I couldn't find.
I'll also look at http://www.mail-archive.com/[e-mail]/msg2193.html, it should be relatively easy to port the Win32 memory stuff to OS/2.
Also we can make a patch for the 1.9.2 branch.
Now it is time for bed :)

Comment 68

[[e-mail]]

FWIW the dlmalloc.c in libffi is v2.8.3, and the latest release is v2.8.4 (ftp://g.oswego.edu/pub/misc/). This may or may not make any difference here (I suspect it won't, since I didn't find any OS2-related stuff on a cursory grep).

Comment 69

[Walter Meinl]

Attachment: patch for 3.0.8 heavily stripped down (checked in on trunk & branch)

Happy New Year. The suggestion to remove the chunk .globl ffi_prep_args
from win32.S enabled us to go completely with windows. Thus, only a tiny patch remains for libffi-3.0.8. With this setup jsctypes-test.js does pass ok.

Comment 70

[Walter Meinl]

(In reply to comment #65)
> Created an attachment (id=419655) [details]
> Proposed patch for upstream
> 
> This builds fine with 3.0.9rc9 downloaded from
> http://github.com/atgreen/libffi.
> Attempts to go totally the win32 route failed due to missing mmap in
> src/dmalloc.c and I couldn't see what was different.
When you use #if (defined(X86_WIN32) && !defined(__OS2__)) || defined(X86_WIN64) in line 47 of closures.c it appears to possible to go the win32 route. 

> Untested due to local build problems. Walter would it be possible for you to
> test this in the Mozilla tree?
Exchanging the in-tree libffi against libffi-3.0.9 (released 2 days ago) and using your attachment results in an unresolved symbol in src/x86/win32.o when xul.dll is linked:
weakld: error: Unresolved symbol (UNDEF) '4'.
weakld: info: The symbol is referenced by:
    E:\mozbuild1\staticlib\components\jsctypes.lib(win32.obj)
(with cp850  looks like a 'club' in card games)
nm shows a "U" not followed by anything beside a U ffi_closure_SYSV_inner in win32.o. It appears something is not properly handled on OS/2 in win32.S patched the way you proposed.

Comment 71

[Dave Yeo]

Don't have much time right now :) Look at the thread libtool and libfii on the newsgroup dated 11/03. I was having the same problem then. Also a shared build of libfii 3.0.9 dies with the same error. Going to have to look at the assembler output I guess

Comment 72

[[e-mail]]

Comment on attachment 419746 [details] [diff] [review]
patch for 3.0.8 heavily stripped down (checked in on trunk & branch)

Wonderful. Drivers: we need this branch-only patch in order to get 1.9.2 building on OS2.

Comment 73

[[e-mail]]

Comment on attachment 412314 [details] [diff] [review]
make ctypes unittests work on OS/2 comment 52 addressed (checked in on trunk & branch)

Need this one on 1.9.2 also.

Comment 74

[Walter Meinl]

(In reply to comment #73)
> (From update of attachment 412314 [details] [diff] [review])
> Need this one on 1.9.2 also.

Dan, this requires that attachment 409553 [details] [diff] [review] is checked-in to 1.9.2 before (yet, it has only been checked in to the branch). BTW, do tests need approval?

Comment 75

[Walter Meinl]

(In reply to comment #74)
> yet, it has only been checked in to the trunk). Sorry for the spam.

Comment 76

[[e-mail]]

Comment on attachment 412314 [details] [diff] [review]
make ctypes unittests work on OS/2 comment 52 addressed (checked in on trunk & branch)

Indeed - tests don't need approval, so this patch won't need it either. We can push all three patches when the third gets approval.

Comment 77

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 419746 [details] [diff] [review]
patch for 3.0.8 heavily stripped down (checked in on trunk & branch)

a192=beltzner, land immediately

Comment 78

[[e-mail]]

Comment on attachment 409553 [details] [diff] [review]
enable ctypes unit tests on all platforms (checked in trunk & branch)

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c4468de3d0ce

Comment 79

[[e-mail]]

Comment on attachment 412314 [details] [diff] [review]
make ctypes unittests work on OS/2 comment 52 addressed (checked in on trunk & branch)

http://hg.mozilla.org/mozilla-central/rev/829fbe79d1bf
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f38c2673beac

Comment 80

[[e-mail]]

Comment on attachment 419746 [details] [diff] [review]
patch for 3.0.8 heavily stripped down (checked in on trunk & branch)

http://hg.mozilla.org/mozilla-central/rev/e8a2b8c6c912
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe55eb993d0f

Comment 81

[[e-mail]]

Fixed on trunk (for now) and 1.9.2. Let's close this bug out and open a new one for getting a libffi 3.0.9 fix on trunk.