652571 - xptstubs on x86_64 does not pass float correctly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mook (work)]

Attachment: fix xptcstubs, resurrect broken test

On Linux64, xptcstubs does not correctly pass the first floats (first 8 with slots shared with doubles, in the XMM registers) correctly.  It instead reads from the stack.

See attached patch, the xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp section.

The attached patch also restores js/src/xpconnect/tests/components/ (which no longer builds because it relies on MOZILLA_INTERNAL_API) to use the external API, and moves the relevant JS test from js/src/xpconnect/tests/js/ to js/src/xpconnect/tests/unit/ (so that it gets run as part of the xpcshell tests).  This means this bug is tested as well as calling C++ from JS.  This does not however fix up all the other tests from js/src/xpconnect/tests/js/.

For reference, the matching (correct) xptcinvoke code is at http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp?rev=a2e48e0c44bb#114

Comment 1

[Benjamin Smedberg]

Is this a recent regression? How come our builds have worked for so long without this patch?

Comment 2

[Mook (work)]

No, this has been around, as far as I can tell, since this initially landed.

Implementing a component in JS with a in float argument (or a float attribute setter) is just not very popular, since for JS it's really just converted to double anyway.  That is: this is a bug in a feature that nobody used :D  (I actually found this doing pyxpcom unit tests...)

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 528125 [details] [diff] [review]
fix xptcstubs, resurrect broken test

Review of attachment 528125 [details] [diff] [review]:

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp
@@ +114,4 @@
                 // The value in %xmm register is already prepared to
                 // be retrieved as a float. Therefore, we pass the
                 // value verbatim, as a double without conversion.
+                dp->val.d =  fpregs[nr_fpr++];

r=me on this part, but I prefer to leave the the test part to someone who might know better... but who? brendan?

Note, as these are really 2 completely different things, I'd be in favour of filing a separate bug for the test issue and keep this one for this one-liner.

Comment 4

[Mook (work)]

Comment on attachment 528125 [details] [diff] [review]
fix xptcstubs, resurrect broken test

Review of attachment 528125 [details] [diff] [review]:
-----------------------------------------------------------------

(This is mostly jotting down notes from timeless that he mentioned on IRC, in the hopes of getting a real r+)
Note to self: de-tabify things as appropriate in a follow-up commit.

Clearing r? pending me fixing those things.

::: js/src/xpconnect/tests/components/xpctest_variant.cpp
@@ +88,5 @@
>  
>  #define MEMBER_COPY_CAST(type_, cast_)                                        \
> +    PR_BEGIN_MACRO                                                            \
> +    cast_ temp;                                                               \
> +    rv = inVar->GetAs##type_( (cast_*) &temp);                                \

timeless mentioned using static_cast here instead (and in the switch() below)

::: js/src/xpconnect/tests/js/readwriteattributes.js
@@ +89,5 @@
>   o.booleanProperty = false;
>   o.shortProperty = -12345;
>   o.longProperty = 1234567890;
>   o.charProperty = "Z";
> + o.floatProperty = 10.2

timeless noted a lack of semicolons here

Comment 5

[Bobby Holley (:bholley)]

Taking, with Mook's permission. Rolling into bug 684327.

Comment 6

[Bobby Holley (:bholley)]

These files were all copy-pasted back in the day, so this is broken in several other places as well (in particular, mac os).

Comment 7

[Bobby Holley (:bholley)]

Attachment: patch v2

Adding an updated patch here, since this affects other platforms as well. The test coverage has been moved over to bug 684327.

Flagging khuey for review.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 561058 [details] [diff] [review]
patch v2

I'm not the correct reviewer for xptcall stuff.

Comment 9

[Bobby Holley (:bholley)]

bsmedberg - this should be pretty easy to review. The old code was just plain wrong, and we have all the test coverage in bug 684327 to prove that the new code is correct. ;-)

Comment 10

[Benjamin Smedberg]

Comment on attachment 561058 [details] [diff] [review]
patch v2

I'm going to bounce this to respindola who has pending patches which touch this code and unify the darwin code. We should probably rebase this on top of those.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Comment on attachment 561058 [details] [diff] [review]
> patch v2
> 
> I'm going to bounce this to respindola who has pending patches which touch
> this code and unify the darwin code. We should probably rebase this on top
> of those.

When is that landing? Bug 683802 depends on bug 684327 which depends on this, and the open web apps team really needs the former landed, hopefully today-ish. It's very close to ready.

Comment 12

[Bobby Holley (:bholley)]

Landed on mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/cba5d081f15d

This passed try as well: https://tbpl.mozilla.org/?tree=Try&rev=82561a8c8594

Comment 13

[Ed Morley [:emorley]]

Backed out of inbound since bholley's push caused xpcshell orange, even after an in-place bustage fix. Was just easier to back the whole push out, sorry!

https://hg.mozilla.org/integration/mozilla-inbound/rev/91b82bc49470

This can likely reland straight away, but just to be sure a green try run would be appreciated. (Comment 12 try run didn't complete on Windows, which was the platform that failed......)

Comment 14

[Ed Morley [:emorley]]

Relanded on inbound :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5622a8f2f9

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0b5622a8f2f9