Bug 227757 – Fix uses of Dependency::fence with respect to the compiler outsmarting us

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

stress/compare-number-and-other.js.ftl-eager: GC Verifier: ERROR cell 0x102e49ba0 was not marked
stress/compare-number-and-other.js.ftl-eager: test_script_33113: line 2: 39217 Trace/BPT trap: 5       ( "$@" ../../.vm/JavaScriptCore.framework/Helpers/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --validateExceptionChecks\=true --useDollarVM\=true --maxPerThreadStackUsage\=1572864 --airForceBriggsAllocator\=true --useRandomizingExecutableIslandAllocation\=true --forcePolyProto\=true --useDataIC\=true --useFTLJIT\=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 compare-number-and-other.js )

Running on an M1 Mac.

[Radar WebKit Bug Importer]

<rdar://problem/80280931>

[Saam Barati]

Been looking into this since yesterday. Adding more logging. Current status is that I'm pretty confused, and not sure what's going on. My logging is indicating something different than what the verifier is saying. Some of my logging is saying an object is marked or that we're marking it, but the verifier is disagreeing

[Saam Barati]

Still getting some weird data. We're seeing a cell that is marked, but then somehow the verifier sees it as unmarked when it look at the normal mark bits.

Need to spend some more time verifying that this is actually what's happening, because it's very peculiar.

[Saam Barati]

Issue seems to go away if I turn off parallel marking constraint solver

[Saam Barati]

(In reply to Saam Barati from comment #4)
> Issue seems to go away if I turn off parallel marking constraint solver

This is wrong. It doesn't go away. But it becomes less common.

[Saam Barati]

What I'm seeing is truly bizarre at this point. Starting to look into the low level details. We're seeing an isMarked check return true then return false for the same object, inside the same function.

Like:

if (visitor.isMarked(this)) {
   // do some things
   RELEASE_ASSERT(visitor.isMarked(this)); // <---- crashes!!!!
}

[Saam Barati]

Created attachment 433916 [details]
patch I'm using to test

[Saam Barati]

The only change needed to apply to ToT to get things to crash is:

Index: Source/JavaScriptCore/bytecode/CodeBlock.cpp
===================================================================
--- Source/JavaScriptCore/bytecode/CodeBlock.cpp	(revision 280124)
+++ Source/JavaScriptCore/bytecode/CodeBlock.cpp	(working copy)
@@ -1214,8 +1214,16 @@ void CodeBlock::determineLiveness(const 
     
 #if ENABLE(DFG_JIT)
     VM& vm = *m_vm;
-    if (visitor.isMarked(this))
+    if (visitor.isMarked(this)) {
+        WTF::compilerFence();
+        if (!visitor.isMarked(this)) {
+            dataLogLn("bit flip, crashing");
+            dataLogLn();
+            RELEASE_ASSERT_NOT_REACHED();
+        }
+
         return;
+    }
     
     // In rare and weird cases, this could be called on a baseline CodeBlock. One that I found was
     // that we might decide that the CodeBlock should be jettisoned due to old age, so the

[Saam Barati]

Created attachment 433981 [details]
patch

[Saam Barati]

Comment on attachment 433981 [details]
patch

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

> Source/WTF/wtf/Atomics.h:439
> +    static Dependency loadAndFence(T* pointer, T& output)

I could also change the API to be pair based instead of using an out parameter.

[Robin Morisset]

Comment on attachment 433981 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:-170
> -JSValue SparseArrayEntry::getConcurrently() const

I'm a bit surprised that you had to remove const here.
Could this be fixed by also having a version of loadAndFence that takes a const T* ?

>> Source/WTF/wtf/Atomics.h:439
>> +    static Dependency loadAndFence(T* pointer, T& output)
> 
> I could also change the API to be pair based instead of using an out parameter.

It would be cleaner, but considering that this is going to be on very hot paths, and my lack of trust in LLVM's abilitiy to optimize a struct return type, I'm in favor of sticking with the out param.

[Saam Barati]

Comment on attachment 433981 [details]
patch

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

>> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:-170
>> -JSValue SparseArrayEntry::getConcurrently() const
> 
> I'm a bit surprised that you had to remove const here.
> Could this be fixed by also having a version of loadAndFence that takes a const T* ?

Good point. I think it'll be as easy as taking 'const T*' in that function.

>>> Source/WTF/wtf/Atomics.h:439
>>> +    static Dependency loadAndFence(T* pointer, T& output)
>> 
>> I could also change the API to be pair based instead of using an out parameter.
> 
> It would be cleaner, but considering that this is going to be on very hot paths, and my lack of trust in LLVM's abilitiy to optimize a struct return type, I'm in favor of sticking with the out param.

ok, I'll leave it as is for now.

[Saam Barati]

Created attachment 434025 [details]
patch for landing

[EWS]

Committed r280195 (239883@main): <https://commits.webkit.org/239883@main>

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