jsctypes busted on linux with gcc 4.5

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
js-ctypes
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: (dormant account), Assigned: glandium)

Tracking

unspecified
mozilla2.0b7
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
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

8 years ago
Haha. In that case, it's probably time to file this upstream with gcc.
(Assignee)

Comment 3

8 years ago
This is probably the same as bug 590683.
(Reporter)

Updated

7 years ago
Summary: jsctypes busted on 64bit linux → jsctypes busted on linux
(Reporter)

Updated

7 years ago
Summary: jsctypes busted on linux → jsctypes busted on linux with gcc 4.5
(Assignee)

Comment 5

7 years ago
Created attachment 475058 [details] [diff] [review]
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.
Assignee: nobody → mh+mozilla
Attachment #475058 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #475058 - Flags: review? → review?(dwitte)

Comment 6

7 years ago
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?
Attachment #475058 - Flags: review?(dwitte) → review+
(Assignee)

Comment 7

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
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.
Attachment #475058 - Flags: approval2.0?

Updated

7 years ago
Attachment #475058 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 10

7 years ago
http://hg.mozilla.org/mozilla-central/rev/aaae8b035859
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7

Comment 11

7 years ago
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. :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla2.0b7 → ---
(Assignee)

Comment 12

7 years ago
(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

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

Comment 14

7 years ago
Created attachment 480046 [details] [diff] [review]
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.
Attachment #480046 - Flags: review?(dwitte)

Comment 15

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

r=dwitte, thanks!
Attachment #480046 - Flags: review?(dwitte) → review+
(Assignee)

Comment 16

7 years ago
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.
Attachment #480046 - Flags: approval2.0?

Comment 17

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

approved for after beta7
Attachment #480046 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 18

7 years ago
http://hg.mozilla.org/mozilla-central/rev/1449562d5013
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.