Bug 227785 - Continue to consult InlineAccess's Structure even after switching to a stub IC
Summary: Continue to consult InlineAccess's Structure even after switching to a stub IC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-07 18:32 PDT by Saam Barati
Modified: 2021-07-10 18:28 PDT (History)
8 users (show)

See Also:


Attachments
patch (4.01 KB, patch)
2021-07-09 13:05 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (18.05 KB, patch)
2021-07-09 15:10 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (18.06 KB, patch)
2021-07-10 13:58 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (18.06 KB, patch)
2021-07-10 13:59 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2021-07-07 18:32:00 PDT
Only observed once. 

stress/class-subclassing-function.js.ftl-eager-no-cjit: test_script_17253: line 2: 37659 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Helpers/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --validateExceptionChecks\=true --useDollarVM\=true --maxPerThreadStackUsage\=1572864 --validateGraph\=true --validateBCE\=true --airForceIRCAllocator\=true --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --thresholdForOMGOptimizeAfterWarmUp\=20 --thresholdForOMGOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 --useEagerCodeBlockJettisonTiming\=true --repatchBufferingCountdown\=0 --collectContinuously\=true --useGenerationalGC\=false --verifyGC\=true --forceOSRExitToLLInt\=true --useExecutableAllocationFuzz\=true --fireExecutableAllocationFuzzRandomly\=true class-subclassing-function.js )


Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000005


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	0x0000000104415b68 0 + 4366359400
1   ???                           	0x0000000104410008 0 + 4366336008
2   ???                           	0x0000000104410008 0 + 4366336008
3   ???                           	0x00000001044107e8 0 + 4366338024
4   com.apple.JavaScriptCore      	0x00000001031311a8 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 76 (JITCodeInlines.h:42) [inlined]
5   com.apple.JavaScriptCore      	0x00000001031311a8 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) + 12052 (Interpreter.cpp:836)
6   com.apple.JavaScriptCore      	0x00000001033ff83c JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 276 (Completion.cpp:137)
7   jsc                           	0x00000001006ebd20 runWithOptions(GlobalObject*, CommandLine&, bool&) + 1540 (jsc.cpp:3068) [inlined]
8   jsc                           	0x00000001006ebd20 jscmain(int, char**)::$_8::operator()(JSC::VM&, GlobalObject*, bool&) const + 1664 (jsc.cpp:3640) [inlined]
9   jsc                           	0x00000001006ebd20 int runJSC<jscmain(int, char**)::$_8>(CommandLine const&, bool, jscmain(int, char**)::$_8 const&) + 2292 (jsc.cpp:3462) [inlined]
10  jsc                           	0x00000001006ebd20 jscmain(int, char**) + 3152 (jsc.cpp:3633)
11  jsc                           	0x00000001006eb074 main + 44 (jsc.cpp:2856)
12  dyld                          	0x00000001009090fc start + 520
Comment 1 Saam Barati 2021-07-08 19:25:26 PDT
We have an inline access that points to Structure S. However, S dies, and we don't clear it.
Comment 2 Saam Barati 2021-07-08 20:25:39 PDT
Seems like the cache type of the SSI is changed from "get by id self" somehow.
Comment 3 Saam Barati 2021-07-09 10:59:27 PDT
Found the bug:

1. Initialize SSI to be an inline self access loading from structure S.
2. We transition to being a PolymorphicAccess based SSI. But, we haven't generated code yet. We're buffered. So we are still running the inline access. But the SSI thinks it's a "Stub".
3. S is collected
4. we continue to run (1)
Comment 4 Saam Barati 2021-07-09 13:05:54 PDT
Created attachment 433235 [details]
patch
Comment 5 Filip Pizlo 2021-07-09 13:18:41 PDT
Comment on attachment 433235 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Stop running inline access code once we switch to being a Stub IC

Doesn't that create a window of time when we're now taking slow path, when previously we would have had a fast path?

Have you measured perf carefully?

> Source/JavaScriptCore/ChangeLog:14
> +        2. We transition to being a PolymorphicAccess based StructureStubInfo. But, we haven't
> +        generated code yet. We're in the buffered state. So we are still running the inline access
> +        from (1).  But the StructureStubInfo thinks it's a "Stub".

This seems like a bad state to be in.  Can we make it so that PolymorphicAccess only starts buffering after it has generated some initial thing?

> Source/JavaScriptCore/ChangeLog:17
> +        4. We continue to run code from (1), because when we finalize the IC during GC, it
> +        doesn't think it's an inline access.

Why not fix the bug by having the GC still look at the InlineAccess IC when we are in this state?  Or maybe forever?  It can reset it if it ever becomes invalid.
Comment 6 Saam Barati 2021-07-09 15:10:18 PDT
Created attachment 433242 [details]
patch
Comment 7 Saam Barati 2021-07-10 13:58:47 PDT
Created attachment 433271 [details]
patch
Comment 8 Saam Barati 2021-07-10 13:59:35 PDT
Created attachment 433273 [details]
patch
Comment 9 Saam Barati 2021-07-10 14:00:15 PDT
Comment on attachment 433235 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:17
>> +        doesn't think it's an inline access.
> 
> Why not fix the bug by having the GC still look at the InlineAccess IC when we are in this state?  Or maybe forever?  It can reset it if it ever becomes invalid.

This is the approach I went with in the new patch.
Comment 10 Yusuke Suzuki 2021-07-10 14:08:32 PDT
Comment on attachment 433273 [details]
patch

r=me
Comment 11 EWS 2021-07-10 18:27:48 PDT
Committed r279813 (239574@main): <https://commits.webkit.org/239574@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433273 [details].
Comment 12 Radar WebKit Bug Importer 2021-07-10 18:28:17 PDT
<rdar://problem/80425870>