This fixes the NYI MOZ_CRASH on ppc64. Passes bug809674 tests on my local builder (POWER9 big endian).

Should note: this implementation is based partially on the AIX64 code and partially on the PPC32 code. It should work on both ABIs but I've only tested on big endian ELFv2 (musl) so far. If testing on ELFv1 (glibc) is desired, I can do that before it lands.

It appears someone else has also implemented this in Bug 1491981 but their patch doesn't look right to me.

Comment on attachment 9017040 [details] [diff] [review] 1475699-ppc64.patch Review of attachment 9017040 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch! If this patch fixes things for you, I'm not going to argue, but some of the logic seems strange to me. Please file a followup bug if you want to investigate further. I don't know enough about the state of PPC64 to say whether ELFv1 should still be tested. My limited understanding is that the ELFv2 ABI fixed up some things around function pointers and invocations and complicated the parameter passing rules for vector types and decimal floating point types, but left the basics that we need here unaddressed. So I'm inclined to say that not testing ELFv1 is OK. ::: xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp @@ +76,5 @@ > nsXPTCMiniVariant* dp = &dispatchParams[i]; > > + if (i == indexOfJSContext) { > + if (iCount < GPR_COUNT) > + iCount++; I understand that this logic mirrors the x86-64 logic, which is incredibly well tested, but this seems very weird: if we're handling the jscontext argument, we're going to increment iCount here, *and* we're going to increment it in the /* integer type or pointer */ block below? That doesn't seem right... @@ +84,4 @@ > > if (!param.IsOut() && type == nsXPTType::T_DOUBLE) { > + if (fpCount < FPR_COUNT) > + dp->val.d = fprData[fpCount++]; These float changes are, in passing, fixing passing floating-point arguments, correct? @@ +135,1 @@ > ap++; This increment looks suspicious to me. Do we really want to increment `ap` every time through the loop, or only the times that we've consumed an integer/pointer-valued argument? That is, for float/double arguments, we shouldn't be incrementing `ap` (well, we should, I guess, if we've overflowed the float gprs?). Otherwise, something like: foo(int a0, int a1, int a2, int a3, int a4, int a5, int a6, /* fill gprs */ double d0, double d1, /* take a couple fprs */ int a7, int a8 /* make sure these are in the correct place */ ) would try to grab `a7` and `a8` from the wrong place, correct? The bare `7` here would also be better as GPR_COUNT.

(In reply to Nathan Froyd [:froydnj] from comment #4) > ::: xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp > @@ +76,5 @@ > > nsXPTCMiniVariant* dp = &dispatchParams[i]; > > > > + if (i == indexOfJSContext) { > > + if (iCount < GPR_COUNT) > > + iCount++; > > I understand that this logic mirrors the x86-64 logic, which is incredibly > well tested, but this seems very weird: if we're handling the jscontext > argument, we're going to increment iCount here, *and* we're going to > increment it in the /* integer type or pointer */ block below? That doesn't > seem right... OK, this was just me being confused. We do indeed want to skip over the jscontext, so this is the correct logic!

Okay, so I ran the *full* test suite, and we need to hold off for a second. You may be right about `ap++` being wrong. js/xpconnect/tests/unit/test_bug809674.js segfaults: #0 0x00003ffff7f48d14 in memset (dest=0x1, c=0, n=4) at src/string/memset.c:14 #1 0x00003ffff0420b70 in nsXPTType::ZeroValue(void*) const (aValue=0x1, this=0x3ffff5ebd9be <xpt::detail::sParams+10554>) at /var/clean/mozppc/mozilla-unified/obj-powerpc64-foxkit-linux-musl/dist/include/xptinfo.h:304 #2 0x00003ffff0420b70 in nsXPCWrappedJSClass::CleanupOutparams(nsXPTMethodInfo const*, nsXPTCMiniVariant*, bool, unsigned char) const (this=0x1004a4200, info=0x3ffff5eb53cc <xpt::detail::sMethods+36112>, nativeParams=0x3fffffffcd98, inOutOnly=true, count=9 '\t') at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedJSClass.cpp:825 #3 0x00003ffff043c8b4 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) (this=0x1004a4200, wrapper=<optimized out>, methodIndex=<optimized out>, info=0x3ffff5eb53cc <xpt::detail::sMethods+36112>, nativeParams=0x3fffffffcd98) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedJSClass.cpp:1184 #4 0x00003fffefae54f4 in PrepareAndDispatch(nsXPTCStubBase*, uint64_t, uint64_t*, uint64_t*, double*) (self=0x1004a4300, methodIndex=<optimized out>, args=<optimized out>, gprData=0x3fffffffcf98, fprData=0x3fffffffcf30) at /var/clean/mozppc/mozilla-unified/xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp:139 #5 0x00003fffefae4024 in SharedStub () at /var/clean/mozppc/mozilla-unified/xpcom/reflect/xptcall/md/unix/xptcstubs_asm_ppc64_linux.S:97 #6 0x00003fffefae3f8c in NS_InvokeByIndex () at /var/clean/mozppc/mozilla-unified/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_ppc64_linux.S:149 #7 0x00003ffff0446950 in CallMethodHelper::Invoke() (this=0x3fffffffd198) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedNative.cpp:1723 #8 0x00003ffff0446950 in CallMethodHelper::Call() (this=0x3fffffffd198) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedNative.cpp:1268 #9 0x00003ffff0436950 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedNative.cpp:1232 This is also causing e10s-enabled anything to crash. Uselessly, all these crashes were just "pipe error (3): Connection reset by peer", so I wasted two full days debugging ipc, before xpcshell-test started crashing reliably enough for me to get a trace: #0 0x006d006f007a002c in () #1 0x00003ffff0408dcc in CallQueryInterface<nsISupports, nsWrapperCache>(nsISupports*, nsWrapperCache**) (aDestination=0x3fffffff80e0, aSource=0x3fffffff8b30) at /var/clean/mozppc/mozilla-unified/obj-powerpc64-foxkit-linux-musl/dist/include/nsISupportsUtils.h:136 #2 0x00003ffff0408dcc in xpcObjectHelper::xpcObjectHelper(nsISupports*, nsWrapperCache*) (aCache=0x0, aObject=0x3fffffff8b30, this=0x3fffffff80d8) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/xpcObjectHelper.h:33 #3 0x00003ffff0408dcc in XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, unsigned int, nsresult*) (d=$JS::NullValue(), s=0x3fffffff8730, type=..., iid=0x3fffffff8438, arrlen=<optimized out>, pErr=0x0) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCConvert.cpp:410 #4 0x00003ffff043c10c in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) (this=0x1000c0b00, wrapper=<optimized out>, methodIndex=<optimized out>, info=0x3ffff5eb8bd4 <xpt::detail::sMethods+50456>, nativeParams=0x3fffffff86f8) at /var/clean/mozppc/mozilla-unified/js/xpconnect/src/XPCWrappedJSClass.cpp:1154 #5 0x00003fffefae54f4 in PrepareAndDispatch(nsXPTCStubBase*, uint64_t, uint64_t*, uint64_t*, double*) (self=0x1000c3a40, methodIndex=<optimized out>, args=<optimized out>, gprData=0x3fffffff88f8, fprData=0x3fffffff8890) at /var/clean/mozppc/mozilla-unified/xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp:139 So I'm going to say "this is mostly working, but not good enough to land". Sorry for that; I'm still getting up to speed with all the new ways to test. (At least I caught it before checkin...)

(In reply to A. Wilcox [:awilfox] from comment #6) > So I'm going to say "this is mostly working, but not good enough to land". > Sorry for that; I'm still getting up to speed with all the new ways to test. > (At least I caught it before checkin...) Just to be clear, I'm happy for you to land this patch just to remove the MOZ_CRASH in this bug, and then deal with whatever breakage might be exposed via that in other bugs--however many bugs are necessary--bugs are cheap, and maybe other ppc64 users will step in to debug other problems along the way. Please don't feel that you have to fix every problem in this bug! If you want to do that, of course, I'm not going to stop you, but I want you know know that there are alternatives. :)

Okay, much better. Now I have the xpconnect test for bug809674 passing; test_params is still failing in testFloat and testDouble. And xpcshell-test/e10s side, I'm down from 63 to just 2 failures (Bug 1415911, and profiler being unavailable on ppc64 causing test_ext_geckoProfiler_schema.js to fail); none of them implicate xptcall! (In reply to Nathan Froyd [:froydnj] from comment #7) > Just to be clear, I'm happy for you to land this patch just to remove the > MOZ_CRASH in this bug, and then deal with whatever breakage might be exposed > via that in other bugs--however many bugs are necessary--bugs are cheap, and > maybe other ppc64 users will step in to debug other problems along the way. That's a good point, but I don't want to land this when the fix was so easy. > Please don't feel that you have to fix every problem in this bug! If you > want to do that, of course, I'm not going to stop you, but I want you know > know that there are alternatives. :) Thank you. I'm much happier with the state of this patch now, but I'm going to see what I can get done with the FP stuff tonight before I commit to landing it in its better-but-not-all-there state.

So it turns out it wasn't *native* float that was broken at all. This revision passes all native tests and xpconnect's native test suite with it is now green on ppc64.

Comment on attachment 9018053 [details] [diff] [review] Support [implicit_jscontext] XPIDL calls on Linux/PPC64. Review of attachment 9018053 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! r=me on this and subsequent revisions. ::: xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp @@ +88,3 @@ > else > + dp->val.d = *(double*) ap++; > + printf("got double: %lf\n", dp->val.d); You may want to remove this printf before having somebody check this in. :)

Same, with debug printfs removed (d'oh). Ready to land from this side.

Okay, I had our PPC team at Adélie look this over and this is an even better version: it fixes the float/double calling properly. test_params now passes in native and js modes.

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/06e5a5bfd05e Support [implicit_jscontext] XPIDL calls on Linux/PPC64.; r=froydnj