Last Comment Bug 652571 - xptstubs on x86_64 does not pass float correctly
: xptstubs on x86_64 does not pass float correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 684327
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-25 10:41 PDT by Mook (as)
Modified: 2011-09-26 07:38 PDT (History)
4 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix xptcstubs, resurrect broken test (14.54 KB, patch)
2011-04-25 10:41 PDT, Mook (as)
no flags Details | Diff | Review
patch v2 (3.31 KB, patch)
2011-09-19 16:03 PDT, Bobby Holley (busy)
respindola: review+
Details | Diff | Review

Description Mook (as) 2011-04-25 10:41:36 PDT
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
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-04-25 11:12:19 PDT
Is this a recent regression? How come our builds have worked for so long without this patch?
Comment 2 Mook (as) 2011-04-25 11:15:52 PDT
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] 2011-05-04 01:07:50 PDT
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 (as) 2011-09-07 11:12:20 PDT
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 (busy) 2011-09-09 13:46:56 PDT
Taking, with Mook's permission. Rolling into bug 684327.
Comment 6 Bobby Holley (busy) 2011-09-13 16:54:37 PDT
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 (busy) 2011-09-19 16:03:45 PDT
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.
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-20 06:50:58 PDT
Comment on attachment 561058 [details] [diff] [review]
patch v2

I'm not the correct reviewer for xptcall stuff.
Comment 9 Bobby Holley (busy) 2011-09-22 09:23:33 PDT
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 [:bsmedberg] 2011-09-22 09:26:53 PDT
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 (busy) 2011-09-22 10:32:52 PDT
(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 (busy) 2011-09-23 14:57:59 PDT
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] 2011-09-23 19:46:48 PDT
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] 2011-09-25 07:39:39 PDT
Relanded on inbound :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5622a8f2f9
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-26 07:38:53 PDT
https://hg.mozilla.org/mozilla-central/rev/0b5622a8f2f9

Note You need to log in before you can comment on or make changes to this bug.