Closed Bug 652571 Opened 10 years ago Closed 10 years ago
xptstubs on x86
_64 does not pass float correctly
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)
Is this a recent regression? How come our builds have worked for so long without this patch?
Attachment #528125 - Flags: review?(benjamin) → review?(mh+mozilla)
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 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?
Taking, with Mook's permission. Rolling into bug 684327.
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
Depends on: 684327
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
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 on attachment 561058 [details] [diff] [review] patch v2 I'm not the correct reviewer for xptcall stuff.
Attachment #561058 - Flags: review?(khuey) → review?(benjamin)
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 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)
(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+
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
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! 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......)
Target Milestone: mozilla9 → ---
Relanded on inbound :-) https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5622a8f2f9
Target Milestone: --- → mozilla9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.