Bug 233408 – FTL's implementation of HasIndexedProperty for InBounds accesses checks the inverse of what it should be checking when exiting by seeing a hole

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Lukas Bernhard]

During differential testing of webkit I found a sample triggering a miscomputation in FTL.

JSC on git commit: 249d5fd6931d
build options:
./Tools/Scripts/build-jsc --jsc-only --release --cmakeargs="-DENABLE_STATIC_JSC=ON -DCMAKE_C_COMPILER='/usr/bin/clang-12' -DCMAKE_CXX_COMPILER='/usr/bin/clang++-12' -DCMAKE_CXX_FLAGS='-O3 -lrt -latomic -fuse-ld=lld'"

Release/bin/jsc --validateOptions=true --useConcurrentJIT=false --useConcurrentGC=false --validateBCE=true --thresholdForJITSoon=1 --thresholdForJITAfterWarmUp=7 --thresholdForOptimizeAfterWarmUp=7 --thresholdForOptimizeAfterLongWarmUp=7 --thresholdForOptimizeSoon=1 --thresholdForFTLOptimizeAfterWarmUp=10 diff.js

diff.js
```
function main() {
    let v17 = {__proto__:[0,0]};
    v17[2] = 4;
        
    let v92 = 0;
    for (let v95 = 0; v95 < 100; v95++) {
        function v103() {
            function v128() {
                v139 = v92++;
            }   
            [0].map(v128);
        }   
        v17.every(v103);
    }   
    print(v139); // 99 w/o FTL, 18 with FLT (also 99 in spidermonkey)
}
main();
```

[Radar WebKit Bug Importer]

<rdar://problem/85787251>

[Saam Barati]

I have a fix.

[Saam Barati]

Created attachment 445343 [details]
patch

[Mark Lam]

Comment on attachment 445343 [details]
patch

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

r=me with fix.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13398
>                  m_out.notZero64(m_out.load64(baseIndex(m_heaps.ArrayStorage_vector, storage, index, m_graph.varArgChild(m_node, 1))));

Shouldn't this be `isZero64` instead?

[Saam Barati]

Comment on attachment 445343 [details]
patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13398
>>                  m_out.notZero64(m_out.load64(baseIndex(m_heaps.ArrayStorage_vector, storage, index, m_graph.varArgChild(m_node, 1))));
> 
> Shouldn't this be `isZero64` instead?

Yes. Now I need to figure out why my test started passing.

[Saam Barati]

Created attachment 445353 [details]
patch for landing

[EWS]

Committed r286278 (244639@main): <https://commits.webkit.org/244639@main>

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