1690152 - ppc64 xptcinvoke doesn't account for situation with GPRs overflowed but FPRs still in registers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Cameron Kaiser [:spectre]]

This is somewhat of a regression since the old code kinda sorta handled this but possibly by accident. Starting with Fx85 these crashes started showing up in ppc64le opt builds:

#0  nsDOMWindowUtils::NodesFromRect(float, float, float, float, float, float, bool, bool, bool, float, nsINodeList**)
    (this=<optimized out>, aX=328, aY=491, aTopSize=1, aRightSize=1, aBottomSize=1, aLeftSize=1, aIgnoreRootScrollFrame=<optimized out>, aFlushLayout=false, aOnlyVisible=true, aVisibleThreshold=1, aReturn=0xaf08596a9b223200)
    at /home/spectre/src/fx85/dom/base/nsDOMWindowUtils.cpp:1264
#1  0x00007ffff0b1290c in NS_InvokeByIndex ()
    at /home/spectre/src/fx85/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_ppc64_linux.S:149
#2  0x00007fffef554e68 in CallMethodHelper::Invoke() (this=0x7fffffffb9e8)
    at /home/spectre/src/fx85/js/xpconnect/src/XPCWrappedNative.cpp:1620
#3  CallMethodHelper::Call() (this=0x7fffffffb9e8)
    at /home/spectre/src/fx85/js/xpconnect/src/XPCWrappedNative.cpp:1176
#4  XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [clone .constprop.0] (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at /home/spectre/src/fx85/js/xpconnect/src/XPCWrappedNative.cpp:1142
#5  0x00007fffef8fc174 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (cx=0x7fffdc14b800, argc=<optimized out>, vp=0x7fff93229390)
    at /home/spectre/src/fx85/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:925
#6  0x00007fffef51eb84 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
    (args=..., reason=<optimized out>, native=<optimized out>, cx=0x7fffdc14b800) at /home/spectre/src/fx85/js/src/vm/Interpreter.cpp:503
#7  js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
    (cx=0x7fffdc14b800, args=..., construct=<optimized out>, reason=<optimized out>) at /home/spectre/src/fx85/js/src/vm/Interpreter.cpp:594
#8  0x00007fffef504248 in js::CallFromStack(JSContext*, JS::CallArgs const&)
    (args=..., cx=<optimized out>)
    at /home/spectre/src/fx85/js/src/vm/Interpreter.cpp:651
#9  Interpret(JSContext*, js::RunState&)
    (cx=cx@entry=0x7fffdc14b800, state=...)
    at /home/spectre/src/fx85/js/src/vm/Interpreter.cpp:3309

Notice the current function, which has 11 parameters, not counting this. Under the PPC64 ABI, the register allocation should be

(r3 = this)
f1 (skip r4)
f2 (skip r5)
f3 (skip r6)
f4 (skip r7)
f5 (skip r8)
f6 (skip r9)
r10
parameter slot 1 on stack
parameter slot 2 on stack
f7 (skip parameter slot 3)
parameter slot 4 on stack

That's not a typo; the outparam should be in parameter slot 4 on the stack because we already overflowed our GPRs by skipping them for all those floats. Thus, while we do still have FPRs to use, the ABI says should still skip parameter slot 3. Unfortunately, the current code puts the outparam pointer in parameter slot 3, so kaboom.

The previous code sort of deals with this; it has if (i >=7) d++ which probably would have covered this issue, but is the wrong fix. The right fix is to also check whether we overflowed GPRs when we assign FPRs. That's the patch coming up.

It only turns up in opt builds and probably only now because some other code now exposes this function directly to XPConnect, which makes the crashes a lot more common.

Comment 1

[Cameron Kaiser [:spectre]]

Attachment: Bug 1690152 - on ppc64 properly skip parameter slots if we overflow GPRs while still having FPRs to burn. r?tcampbell

Comment 2

[Cameron Kaiser [:spectre]]

@tcampbell do you think we need something similar in xpcom/reflect/xptcall/md/unix/xptcstubs_ppc64_linux.cpp as well? I was dithering over this but I'm not sure it's directly exposed to the stack frame. I don't have a test case for that, but we didn't have one for this situation either.

Comment 3

[Ted Campbell [:tcampbell]]

Good point. We should do the same change there.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/579a66fd7966
on ppc64 properly skip parameter slots if we overflow GPRs while still having FPRs to burn. r=tcampbell

Comment 5

[Cameron Kaiser [:spectre]]

Thanks for the review!

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/579a66fd7966

Comment 7

[Cameron Kaiser [:spectre]]

Comment on attachment 9200560 [details]
Bug 1690152 - on ppc64 properly skip parameter slots if we overflow GPRs while still having FPRs to burn. r?tcampbell

Beta/Release Uplift Approval Request

• User impact if declined: Random crashes on opt builds for this architecture.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): NPOTB.
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Random crashes on opt builds for this architecture. Although the crash frequency is less on versions prior to 85, it can still occur or be triggered by unrelated code changes.
• User impact if declined: Random crashes on opt builds for this architecture.
• Fix Landed on Version: 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): NPOTB.
• String or UUID changes made by this patch: None.

Comment 8

[Dan Horák]

for the record - I have verified the fix with patched FF 85 in Fedora, where the problem has been first discovered.

Comment 9

[Pascal Chevrel:pascalc]

Comment on attachment 9200560 [details]
Bug 1690152 - on ppc64 properly skip parameter slots if we overflow GPRs while still having FPRs to burn. r?tcampbell

NPOTB, Approved for 86 beta 6 and ESR.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e04c3840e84e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/2ffdb46e8a49