[JSC] Do not skip the FPR path if either payload or tag GPR is unavailable

[emutavchi]

This is a potential fix for the issue #1592

[Copilot]

Pull request overview

This PR fixes a critical bug in the DFG JIT compiler for 32-bit architectures (JSVALUE32_64) that caused crashes during tail call optimization. The issue occurred when the code incorrectly decided to skip the FPR (floating-point register) loading path if either the payload GPR or tag GPR was available individually, when both registers are actually required for a complete JSValue in the 32-bit representation.

• Fixed the logic to only skip FPR path when both tag and payload GPRs are available, not just one
• Refactored the availability check into a lambda function for code clarity

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[justinmichaud]

We can only reach the crashing assertion if tryFPR is resultFPR is invalid, otherwise we must return. In that case, tryFPR no longer matters, right? I don't see how this could fix the crash, but let me look deeper into it to see if I am missing something.

In addition, my understanding is that tryFPR is just a fast path to help us get extra registers. So it should always be correct to set tryFPR = false, even if not optimal.

    if (payloadGPR == InvalidGPRReg || m_registers[payloadGPR] || m_lockedRegisters.contains(payloadGPR, IgnoreVectors))
        payloadGPR = getFreeGPR();
    m_lockedRegisters.add(payloadGPR, IgnoreVectors);
    if (tagGPR == InvalidGPRReg || m_registers[tagGPR] || m_lockedRegisters.contains(tagGPR, IgnoreVectors))
        tagGPR = getFreeGPR();
    m_lockedRegisters.remove(payloadGPR);

This looks kinda sketchy, it seems like it is totally possible to get payloadGPR == tagGPR. That would be my guess.

1. This code is better, so either way, this is a good patch. Thank you!
2. But: it seems to be hiding a deeper bug, so we shouldn't land it until we fix that bug
3. We can safely disable tail call optimization on 32-bit. It doesn't add much, and increases the QA burden. I would recommend this as the fastest and most stable fix for now. What do you think?

[emutavchi]

Hi @justinmichaud, thanks for looking into this.

There appears to be some inconsistency in the assumptions made by CallFrameShuffler in canLoad(), ensureLoad(), and emitLoad() on arm32. See, existing code calls canLoad() in ensureLoad() just before emitLoad().

The canLoad() checks if FPR registry is free for cachedRecovery that uses DisplacedInJSStack technique. If one is not available, then it frees up an FPR register. However, emitLoad() skips the FPR code path if either the tag or the payload register is available, even though it requires both registers for DisplacedInJSStack recovery. This leads to a crash when there is only 1 free register. When the tagGPR is free, but payloadGPR is not, the tagGPR that gets "stolen" for payloadGPR. Leaving tagGPR with InvalidGPRReg.