594611 - jsctypes busted on linux with gcc 4.5

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[(dormant account)]

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

Comment 1

[(dormant account)]

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

Comment 2

[[e-mail]]

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

Comment 3

[Mike Hommey [:glandium]]

This is probably the same as bug 590683.

Comment 4

[(dormant account)]

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: Fix stack allocation for ffi function calls on x86-64

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 6

[[e-mail]]

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?

Comment 7

[Mike Hommey [:glandium]]

(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 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[[e-mail]]

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. :(

Comment 12

[Mike Hommey [:glandium]]

(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.

Comment 13

[[e-mail]]

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

Comment 14

[Mike Hommey [:glandium]]

Attachment: Apply upstream patch

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 15

[[e-mail]]

Comment on attachment 480046 [details] [diff] [review]
Apply upstream patch

r=dwitte, thanks!

Comment 16

[Mike Hommey [:glandium]]

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 17

[Benjamin Smedberg]

Comment on attachment 480046 [details] [diff] [review]
Apply upstream patch

approved for after beta7

Comment 18

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/1449562d5013