Bug 230241 - Fix crash in 32 bits due to not enough scratch registers available
Summary: Fix crash in 32 bits due to not enough scratch registers available
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 230215 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-09-13 17:53 PDT by Mikhail R. Gadelha
Modified: 2021-09-16 03:40 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.07 KB, patch)
2021-09-13 18:11 PDT, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2021-09-14 14:09 PDT, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (6.07 KB, patch)
2021-09-14 14:19 PDT, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2021-09-15 10:59 PDT, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail R. Gadelha 2021-09-13 17:53:59 PDT
Fix crash in 32 bits due to not enough scratch registers available
Comment 1 Mikhail R. Gadelha 2021-09-13 18:11:59 PDT
Created attachment 438093 [details]
Patch
Comment 2 Mikhail R. Gadelha 2021-09-13 18:14:22 PDT
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 ?? ()
Comment 3 Mikhail R. Gadelha 2021-09-13 18:22:27 PDT
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.
Comment 4 Xan Lopez 2021-09-14 05:29:52 PDT
*** Bug 230215 has been marked as a duplicate of this bug. ***
Comment 5 Caio Lima 2021-09-14 06:24:35 PDT
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.
Comment 6 Mikhail R. Gadelha 2021-09-14 14:09:07 PDT
Created attachment 438168 [details]
Patch
Comment 7 Mikhail R. Gadelha 2021-09-14 14:16:44 PDT
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.
Comment 8 Mikhail R. Gadelha 2021-09-14 14:19:58 PDT
Created attachment 438171 [details]
Patch
Comment 9 Caio Lima 2021-09-15 06:03:58 PDT
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.
Comment 10 Mikhail R. Gadelha 2021-09-15 08:01:11 PDT
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
Comment 11 Mikhail R. Gadelha 2021-09-15 10:59:21 PDT
Created attachment 438260 [details]
Patch
Comment 12 Caio Lima 2021-09-15 11:19:10 PDT
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.
Comment 13 EWS 2021-09-16 03:39:18 PDT
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].
Comment 14 Radar WebKit Bug Importer 2021-09-16 03:40:16 PDT
<rdar://problem/83190749>