xptstubs on x86_64 does not pass float correctly

RESOLVED FIXED in mozilla9



8 years ago
7 years ago


(Reporter: mark.yen, Assigned: bholley)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



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

Comment 1

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


8 years ago
Attachment #528125 - Flags: review?(benjamin) → review?(mh+mozilla)

Comment 2

8 years ago
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 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.
Attachment #528125 - Flags: review?(mh+mozilla) → review?

Comment 4

7 years ago
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
Attachment #528125 - Flags: review?

Comment 5

7 years ago
Taking, with Mook's permission. Rolling into bug 684327.
Assignee: nobody → bobbyholley+bmo
Depends on: 684327

Comment 6

7 years ago
These files were all copy-pasted back in the day, so this is broken in several other places as well (in particular, mac os).
Summary: xptstubs on linux64 does not pass float correctly → xptstubs on x86_64 does not pass float correctly

Comment 7

7 years ago
Created attachment 561058 [details] [diff] [review]
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.
Attachment #528125 - Attachment is obsolete: true
Attachment #561058 - Flags: review?(khuey)
Comment on attachment 561058 [details] [diff] [review]
patch v2

I'm not the correct reviewer for xptcall stuff.
Attachment #561058 - Flags: review?(khuey) → review?(benjamin)

Comment 9

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

7 years ago
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.
Attachment #561058 - Flags: review?(benjamin) → review?(respindola)

Comment 11

7 years ago
(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.
Attachment #561058 - Flags: review?(respindola) → review+

Comment 12

7 years ago
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
Flags: in-testsuite+
Target Milestone: --- → mozilla9
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!


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......)
Target Milestone: mozilla9 → ---
Relanded on inbound :-)

Target Milestone: --- → mozilla9

Comment 15

7 years ago
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.