TEST-UNEXPECTED-FAIL | /home/taras/builds/minefield.451/_tests/xpcshell/toolkit/components/ctypes/tests/unit/test_jsctypes.js | false == true - See following stack: JS frame :: /home/taras/work/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 317 JS frame :: /home/taras/work/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 347 JS frame :: /home/taras/builds/minefield.451/_tests/xpcshell/toolkit/components/ctypes/tests/unit/test_jsctypes.js :: run_single_abi_tests :: line 705 JS frame :: /home/taras/builds/minefield.451/_tests/xpcshell/toolkit/components/ctypes/tests/unit/test_jsctypes.js :: run_basic_abi_tests :: line 660 JS frame :: /home/taras/builds/minefield.451/_tests/xpcshell/toolkit/components/ctypes/tests/unit/test_jsctypes.js :: run_bool_tests :: line 832 JS frame :: /home/taras/builds/minefield.451/_tests/xpcshell/toolkit/components/ctypes/tests/unit/test_jsctypes.js :: run_test :: line 102 JS frame :: /home/taras/work/mozilla-central/testing/xpcshell/head.js :: _execute_test :: line 203 JS frame :: -e :: <TOP_LEVEL> :: line 1 TEST-INFO | (xpcshell/head.js) | exiting test

Ok, Good news. Trunk gcc passes this with flying colours. Bad news is that now someone was to figure out a fix to backport.

Haha. In that case, it's probably time to file this upstream with gcc.

This is probably the same as bug 590683.

filed http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45623

It turns out this is not the same as bug 590683, and is a libffi bug that some gcc optimizations unveil. The issue is actually reproducible with gcc-4.4 with various compile flags such as "-O2 -fno-inline". You actually only need to build libjsctypes-test.so with these flags. The problem is a quite subtle cornercase: the stack space allocated for the non-register arguments is not big enough (and not properly aligned) when calling the target function, and depending on what the called function does with the stack, it can end up overwriting ffi_call_unix64's stack. In the present case, depending on gcc optimization, here is what can happen on the stack in the sum_many_bool_cdecl function: movzbl 0x60(%rsp),%eax mov %eax,0x60(%rsp) The problem here is that at %rsp + 0x5f are stored the flags. They therefore end up being overwritten and after the function returns, flags are just NULL, so the return value is considered as FFI_TYPE_VOID and is thus not stored at the return address. So all in all the problem is that the number of bytes allocated on stack for the function call, as allocated by ffi_call is not enough.

Comment on attachment 475058 [details] [diff] [review] Fix stack allocation for ffi function calls on x86-64 Nice! r=dwitte. We need to upstream this. I can push it to the list if you'd like. In the meantime, we want to take this locally. Can you copy-paste this diff into js/src/ctypes/libffi.patch, and reference this bug# at the top?

(In reply to comment #6) > In the meantime, we want to take this locally. Can you copy-paste this diff > into js/src/ctypes/libffi.patch, and reference this bug# at the top? I will when landing.

Comment on attachment 475058 [details] [diff] [review] Fix stack allocation for ffi function calls on x86-64 This breaks ctypes on linux x86-64 in some cornercases.

Filed http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

It looks like we need a better fix here. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677#c4. Sorry I didn't catch this in review. :(

(In reply to comment #11) > It looks like we need a better fix here. See > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677#c4. > > Sorry I didn't catch this in review. :( Sorry I didn't either. 6 eyes are better than 4. Fortunately, it's only going to break if using arguments wider than 64 bits, so the patch as it is isn't going to break anything immediately.

Yeah. It'll only break on long double (or structs using long double), which we don't implement in ctypes.

This applies on top of what we now have on m-c to fit upstream patch as committed on gcc tree. I left the useless space change as is because this should make later merging easier.

Comment on attachment 480046 [details] [diff] [review] Apply upstream patch r=dwitte, thanks!

Comment on attachment 480046 [details] [diff] [review] Apply upstream patch It would be better to apply the patch as it is upstream, even if the patch currently on m-c doesn't break anything.

Comment on attachment 480046 [details] [diff] [review] Apply upstream patch approved for after beta7