Bug 231321 - EnumeratorGetByVal for IndexedMode+OwnStructureMode doesn't always recover the property name
Summary: EnumeratorGetByVal for IndexedMode+OwnStructureMode doesn't always recover th...
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-10-06 12:40 PDT by Lukas Bernhard
Modified: 2021-11-02 10:25 PDT (History)
9 users (show)

See Also:


Attachments
patch (10.39 KB, patch)
2021-11-01 15:54 PDT, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (10.46 KB, patch)
2021-11-01 16:33 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 Lukas Bernhard 2021-10-06 12:40:48 PDT
Differential testing identifies the following samples to trigger a miscomputation in FTL.
Tested on e467a9710432ebb3dae9880f897cf93929adc0e6 (Wed Oct 6 16:30:57 2021 +0000)
Sorry I couldn't minimize the testcase further, everything I try to simplify breaks the differential behavior.
Also, the bug description is obviously meaningless due to not having a hunch regarding the root cause.

Release/bin/jsc --validateOptions=true --useConcurrentJIT=false --useConcurrentGC=false --thresholdForJITSoon=10 --thresholdForJITAfterWarmUp=10 --thresholdForOptimizeAfterWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForOptimizeSoon=100 --thresholdForFTLOptimizeAfterWarmUp=1000 --thresholdForFTLOptimizeSoon=1000 --validateBCE=true --useFTLJIT=true diff.js

function main() {
    let v224;
    const v35 = [0, 0, {b:"AAAAA"}];
    
    async function v36(arr) {
        edenGC();  // removing this break differential
        for (let v198 = 0; v198 < 2; v198++) {
            const v200 = [0, 0]; 
            const v201 = ` 
                for (let v205 = 0; v205 < 60000; v205++) { }

                async function v215() { } // never called but removing breaks differential

                const v222 = {"__proto__":[[]], "a":0, "b":0};
                for (const v223 in v222) {
                    v224 = arr[v223];
                    v222.__proto__ = {};
                }
                v200;
            `;
            eval(v201); // moving code out of eval breaks differential
        }   
    }   
    v35.filter(v36);
    print(v224) // prints undefined in FTL, AAAAA without FTL (also AAAAA in v8)
}
main();
Comment 1 Radar WebKit Bug Importer 2021-10-13 12:41:16 PDT
<rdar://problem/84211697>
Comment 2 Saam Barati 2021-11-01 15:54:36 PDT
Created attachment 443027 [details]
patch
Comment 3 Yusuke Suzuki 2021-11-01 16:06:04 PDT
Comment on attachment 443027 [details]
patch

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:15908
> +            MacroAssembler::JumpList& genericOrRecoverCase = m_graph.varArgChild(node, 1).node() == m_graph.varArgChild(node, 3).node() ? recoverGenericCase : notFastNamedCases;

For readability, let's define `bool indexedModeAndOwnStructureMode = ...` and use it.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13686
> +        // FIXME: This is a pretty bad way to say we're using IndexedMode+OwnStructureMode mode. 

Ditto for bool variable.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13755
> +        if (indexEdge.node() == propertyNameEdge.node()) {

Use bool variable which should be defined above.
Comment 4 Saam Barati 2021-11-01 16:33:33 PDT
Created attachment 443034 [details]
patch for landing
Comment 5 EWS 2021-11-02 10:25:53 PDT
Committed r285167 (243803@main): <https://commits.webkit.org/243803@main>

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