Bug 230241 – Fix crash in 32 bits due to not enough scratch registers available

• Component: New Bugs
• Status: RESOLVED FIXED

[Mikhail R. Gadelha]

Fix crash in 32 bits due to not enough scratch registers available

[Mikhail R. Gadelha]

Created attachment 438093 [details]
Patch

[Mikhail R. Gadelha]

The bt:
#0  __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
#1  0xf5e76ea0 in __libc_signal_restore_set (set=0xfffecc7c) at ../sysdeps/unix/sysv/linux/internal-signals.h:86
#2  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
#3  0xf5e677a2 in __GI_abort () at abort.c:79
#4  0xf6cc52ca in JSC::ScratchRegisterAllocator::allocateScratch<JSC::GPRInfo> (this=0xfffecfa0) at ../../Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp:98
#5  0xf6cbc120 in JSC::ScratchRegisterAllocator::allocateScratchGPR (this=0xfffecfa0) at ../../Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp:103
#6  0xf62dd070 in JSC::AccessCase::generateImpl (this=0xf36b1cc0, state=...) at ../../Source/JavaScriptCore/bytecode/AccessCase.cpp:2277
#7  0xf62dafda in JSC::AccessCase::generateWithGuard (this=0xf36b1cc0, state=..., fallThrough=...) at ../../Source/JavaScriptCore/bytecode/AccessCase.cpp:1755
#8  0xf63bb07a in JSC::PolymorphicAccess::regenerate (this=0xf36b1ca0, locker=..., vm=..., globalObject=0xf20f9038, codeBlock=0xf1e98000, ecmaMode=..., stubInfo=...)
    at ../../Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:672
#9  0xf63e30c4 in operator() (__closure=0xfffee5f4) at ../../Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:221
#10 0xf63e321e in JSC::StructureStubInfo::addAccessCase (this=0xf36f6840, locker=..., globalObject=0xf20f9038, codeBlock=0xf1e98000, ecmaMode=..., ident=..., accessCase=...)
    at ../../Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:245
#11 0xf6cb775e in JSC::tryCachePutBy (globalObject=0xf20f9038, codeBlock=0xf1e98000, baseValue=..., oldStructure=0xf1eb4be0, propertyName=..., slot=..., stubInfo=..., putByKind=JSC::PutByKind::ByVal, 
    putKind=JSC::PutKind::NotDirect) at ../../Source/JavaScriptCore/jit/Repatch.cpp:877
#12 0xf6cb791e in JSC::repatchPutBy (globalObject=0xf20f9038, codeBlock=0xf1e98000, baseValue=..., oldStructure=0xf1eb4be0, propertyName=..., slot=..., stubInfo=..., putByKind=JSC::PutByKind::ByVal, 
    putKind=JSC::PutKind::NotDirect) at ../../Source/JavaScriptCore/jit/Repatch.cpp:897
#13 0xf6c5b364 in JSC::putByValOptimize (globalObject=0xf20f9038, codeBlock=0xf1e98000, baseValue=..., subscript=..., value=..., stubInfo=0xf36f6840, profile=0xf36d30d0, ecmaMode=...)
    at ../../Source/JavaScriptCore/jit/JITOperations.cpp:1055
#14 0xf6c5b516 in JSC::operationPutByValNonStrictOptimize (globalObject=0xf20f9038, encodedBaseValue=-17390867880, encodedSubscript=-17415805264, encodedValue=-4294967295, stubInfo=0xf36f6840, 
    profile=0xf36d30d0) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:1087
#15 0xf2600e68 in ?? ()

[Mikhail R. Gadelha]

I was reading the code in AccessCode.cpp and noticed that there is a check if there are enough registers available in AccessCase::createTransition before calling the code that triggers the error, so maybe the best fix is to improve AccessCase::createTransition so it doesn't allow the fast path if there isn't enough registers available?

Current the check is: 

    // Skip optimizing the case where we need a realloc, if we don't have
    // enough registers to make it happen.
    if (GPRInfo::numberOfRegisters < 6
        && oldStructure->outOfLineCapacity() != newStructure->outOfLineCapacity()
        && oldStructure->outOfLineCapacity()) {
        return nullptr;
    }

Maybe change it to allocator.numberOfSetRegisters()? We don't have any allocator there yet though.

[Xan Lopez]

*** Bug 230215 has been marked as a duplicate of this bug. ***

[Caio Lima]

Comment on attachment 438093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438093&action=review

> Source/JavaScriptCore/bytecode/AccessCase.cpp:2327
> +                slowPath.append(jit.jump());

I’m not comfortable with this generating a bunch of IC code for checks and other things to always jump to slow path. This seems quite suboptimal. Isn’t there a way were we skip a case if we figure out we don’t have enough registers for it? Skipping such case will result in always taking the slow path as well, but we don’t emit any unnecessary code for it.

[Mikhail R. Gadelha]

Created attachment 438168 [details]
Patch

[Mikhail R. Gadelha]

The following RELEASE_ASSERT doesn't make too much sense after this patch:

    case Transition: {
        ASSERT(!viaProxy());
        // AccessCase::createTransition() should have returned null if this wasn't true.
        RELEASE_ASSERT(GPRInfo::numberOfRegisters >= 6 || !structure()->outOfLineCapacity() || structure()->outOfLineCapacity() == newStructure()->outOfLineCapacity());

since the number of required registers depends on the arch, if we are allocating, reallocating, or doing either inline (basically the new logic in AccessCase::createTransition).

Should we keep this RELEASE_ASSERT like that? I think 6 is the bare minimum required registers but it doesn't say much.

[Mikhail R. Gadelha]

Created attachment 438171 [details]
Patch

[Caio Lima]

Comment on attachment 438171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438171&action=review

I like this version much more. I’m just a bit afraid we are giving up on cache on cases where we still could do it. Could you run some benchmarks to verify if we are not regressing anything?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:140
> +        requiredRegisters *= 2;

This is a bit conservative, because we can have `baseRegs` be only payload register, since we know somehow it’s a cell. IIRC, such info will be available on stubInfo.

[Mikhail R. Gadelha]

Unfortunately, I couldn't use JetStream2 to check for performance regression in 32 bits, since jsc crashes due to not having enough registers :/

For x86_64 I got:

a mean = 150.98901
b mean = 149.40733
pValue = 0.8222857950
(Bigger means are better.)
1.011 times worse
Results ARE NOT significant

[Mikhail R. Gadelha]

Created attachment 438260 [details]
Patch

[Caio Lima]

Comment on attachment 438260 [details]
Patch

Patch LGTM.  If possible, it would be nice verify if there’s no regression with run-jsc-benchmarks as well. It’s unlikely that this will regress things, but better safe than sorry about performance regression.

[EWS]

Committed r282540 (241742@main): <https://commits.webkit.org/241742@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438260 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83190749>