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
Summary: FTL's implementation of HasIndexedProperty for InBounds accesses checks the i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: PC Linux
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-21 06:24 PST by Lukas Bernhard
Modified: 2021-11-29 17:58 PST (History)
9 users (show)

See Also:


Attachments
patch (10.68 KB, patch)
2021-11-29 14:21 PST, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (12.63 KB, patch)
2021-11-29 15:12 PST, 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 Lukas Bernhard 2021-11-21 06:24:53 PST
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();
```
Comment 1 Radar WebKit Bug Importer 2021-11-28 06:25:21 PST
<rdar://problem/85787251>
Comment 2 Saam Barati 2021-11-29 13:57:47 PST
I have a fix.
Comment 3 Saam Barati 2021-11-29 14:21:59 PST
Created attachment 445343 [details]
patch
Comment 4 Mark Lam 2021-11-29 14:30:12 PST
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?
Comment 5 Saam Barati 2021-11-29 14:47:10 PST
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.
Comment 6 Saam Barati 2021-11-29 15:12:06 PST
Created attachment 445353 [details]
patch for landing
Comment 7 EWS 2021-11-29 17:58:45 PST
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].