⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 243278 in webkit


Ignore:
Timestamp:
Mar 20, 2019, 10:43:08 PM (7 years ago)
Author:
sbarati@apple.com
Message:

DFG::AbstractValue::validateOSREntry is wrong when isHeapTop and the incoming value is Empty
​https://bugs.webkit.org/show_bug.cgi?id=195721

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

There was a check in AbstractValue::validateOSREntry where it checked
if isHeapTop(), and if so, just returned true. However, this is wrong
if the value we're checking against is the empty value, since HeapTop
does not include the Empty value. Instead, this check should be
isBytecodeTop(), which does account for the empty value.

This patch also does a couple of other things:

  • For our OSR entry AbstractValues, we were using HeapTop to mark a dead value. That is now changed to BytecodeTop. (The idea here is just to have validateOSREntry return early.)
  • It wasn't obvious to me how I could make this fail in JS code. The symptom we'd end up seeing is something like a nullptr derefernece from forgetting to do a TDZ check. Instead, I've added a unit test. This unit test lives in a new test file: testdfg. testdfg is similar to testb3/testair/testapi.
  • JavaScriptCore.xcodeproj/project.pbxproj:
  • bytecode/SpeculatedType.h:
  • dfg/DFGAbstractValue.h:

(JSC::DFG::AbstractValue::isBytecodeTop const):
(JSC::DFG::AbstractValue::validateOSREntryValue const):

  • dfg/testdfg.cpp: Added.

(hiddenTruthBecauseNoReturnIsStupid):
(usage):
(JSC::DFG::testEmptyValueDoesNotValidateWithHeapTop):
(JSC::DFG::run):
(run):
(main):

  • shell/CMakeLists.txt:

Tools:

  • Scripts/run-javascriptcore-tests:
Location:
trunk
Files:
1 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r243277 r243278  
     12019-03-20  Saam Barati  <sbarati@apple.com>
     2
     3        DFG::AbstractValue::validateOSREntry is wrong when isHeapTop and the incoming value is Empty
     4        https://bugs.webkit.org/show_bug.cgi?id=195721
     5
     6        Reviewed by Filip Pizlo.
     7
     8        There was a check in AbstractValue::validateOSREntry where it checked
     9        if isHeapTop(), and if so, just returned true. However, this is wrong
     10        if the value we're checking against is the empty value, since HeapTop
     11        does not include the Empty value. Instead, this check should be
     12        isBytecodeTop(), which does account for the empty value.
     13       
     14        This patch also does a couple of other things:
     15        - For our OSR entry AbstractValues, we were using HeapTop to mark
     16         a dead value. That is now changed to BytecodeTop. (The idea here
     17         is just to have validateOSREntry return early.)
     18        - It wasn't obvious to me how I could make this fail in JS code.
     19         The symptom we'd end up seeing is something like a nullptr derefernece
     20         from forgetting to do a TDZ check. Instead, I've added a unit test.
     21         This unit test lives in a new test file: testdfg. testdfg is similar
     22         to testb3/testair/testapi.
     23
     24        * JavaScriptCore.xcodeproj/project.pbxproj:
     25        * bytecode/SpeculatedType.h:
     26        * dfg/DFGAbstractValue.h:
     27        (JSC::DFG::AbstractValue::isBytecodeTop const):
     28        (JSC::DFG::AbstractValue::validateOSREntryValue const):
     29        * dfg/testdfg.cpp: Added.
     30        (hiddenTruthBecauseNoReturnIsStupid):
     31        (usage):
     32        (JSC::DFG::testEmptyValueDoesNotValidateWithHeapTop):
     33        (JSC::DFG::run):
     34        (run):
     35        (main):
     36        * shell/CMakeLists.txt:
     37
    1382019-03-20  Saam Barati  <sbarati@apple.com>
    239

  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r243254 r243278  
    6060                        );
    6161                        dependencies = (
     62                                52CD0F662242F5A3004A18A5 /* PBXTargetDependency */,
    6263                                7954BE0D20B63348009BC83A /* PBXTargetDependency */,
    6364                                FE533CAF1F217EC60016A1FE /* PBXTargetDependency */,
    … …  
    885886                52C0611F1AA51E1C00B4ADBA /* RuntimeType.h in Headers */ = {isa = PBXBuildFile; fileRef = 52C0611D1AA51E1B00B4ADBA /* RuntimeType.h */; settings = {ATTRIBUTES = (Private, ); }; };
    886887                52C952B719A289850069B386 /* TypeProfiler.h in Headers */ = {isa = PBXBuildFile; fileRef = 52C952B619A289850069B386 /* TypeProfiler.h */; settings = {ATTRIBUTES = (Private, ); }; };
     888                52CD0F5D2242F569004A18A5 /* Foundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 51F0EB6105C86C6B00E6DF1B /* Foundation.framework */; };
     889                52CD0F5E2242F569004A18A5 /* JavaScriptCore.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 932F5BD90822A1C700736975 /* JavaScriptCore.framework */; };
     890                52CD0F682242F71C004A18A5 /* testdfg.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 52CD0F672242F71C004A18A5 /* testdfg.cpp */; };
    887891                52D13091221CE176009C836C /* foo.js in Copy Support Script */ = {isa = PBXBuildFile; fileRef = 52D1308F221CE03A009C836C /* foo.js */; };
    888892                52F6C35E1E71EB080081F4CC /* WebAssemblyWrapperFunction.h in Headers */ = {isa = PBXBuildFile; fileRef = 52F6C35C1E71EB080081F4CC /* WebAssemblyWrapperFunction.h */; };
    … …  
    19251929                        remoteGlobalIDString = 65FB3F6609D11E9100F49DEB;
    19261930                        remoteInfo = "Derived Sources";
     1931                };
     1932                52CD0F652242F5A3004A18A5 /* PBXContainerItemProxy */ = {
     1933                        isa = PBXContainerItemProxy;
     1934                        containerPortal = 0867D690FE84028FC02AAC07 /* Project object */;
     1935                        proxyType = 1;
     1936                        remoteGlobalIDString = 52CD0F592242F569004A18A5;
     1937                        remoteInfo = testdfg;
    19271938                };
    19281939                53B4BD131F68C2AA00D2BEA3 /* PBXContainerItemProxy */ = {
    … …  
    33823393                52C952B619A289850069B386 /* TypeProfiler.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TypeProfiler.h; sourceTree = "<group>"; };
    33833394                52C952B819A28A1C0069B386 /* TypeProfiler.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = TypeProfiler.cpp; sourceTree = "<group>"; };
     3395                52CD0F642242F569004A18A5 /* testdfg */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = testdfg; sourceTree = BUILT_PRODUCTS_DIR; };
     3396                52CD0F672242F71C004A18A5 /* testdfg.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = testdfg.cpp; path = dfg/testdfg.cpp; sourceTree = "<group>"; };
    33843397                52D1308F221CE03A009C836C /* foo.js */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.javascript; path = foo.js; sourceTree = "<group>"; };
    33853398                52F6C35B1E71EB080081F4CC /* WebAssemblyWrapperFunction.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = WebAssemblyWrapperFunction.cpp; path = js/WebAssemblyWrapperFunction.cpp; sourceTree = "<group>"; };
    … …  
    50015014                        runOnlyForDeploymentPostprocessing = 0;
    50025015                };
     5016                52CD0F5C2242F569004A18A5 /* Frameworks */ = {
     5017                        isa = PBXFrameworksBuildPhase;
     5018                        buildActionMask = 2147483647;
     5019                        files = (
     5020                                52CD0F5D2242F569004A18A5 /* Foundation.framework in Frameworks */,
     5021                                52CD0F5E2242F569004A18A5 /* JavaScriptCore.framework in Frameworks */,
     5022                        );
     5023                        runOnlyForDeploymentPostprocessing = 0;
     5024                };
    50035025                651122FC14046A4C002B101D /* Frameworks */ = {
    50045026                        isa = PBXFrameworksBuildPhase;
    … …  
    50695091                                6511230514046A4C002B101D /* testRegExp */,
    50705092                                932F5BD90822A1C700736975 /* JavaScriptCore.framework */,
     5093                                52CD0F642242F569004A18A5 /* testdfg */,
    50715094                        );
    50725095                        name = Products;
    … …  
    76477670                                0FDB2CE6174830A2007B3C1B /* DFGWorklist.h */,
    76487671                                0F1FB3951E1AF7DF00A9BE50 /* DFGWorklistInlines.h */,
     7672                                52CD0F672242F71C004A18A5 /* testdfg.cpp */,
    76497673                        );
    76507674                        name = dfg;
    … …  
    1003910063                        productType = "com.apple.product-type.tool";
    1004010064                };
     10065                52CD0F592242F569004A18A5 /* testdfg */ = {
     10066                        isa = PBXNativeTarget;
     10067                        buildConfigurationList = 52CD0F5F2242F569004A18A5 /* Build configuration list for PBXNativeTarget "testdfg" */;
     10068                        buildPhases = (
     10069                                52CD0F5A2242F569004A18A5 /* Sources */,
     10070                                52CD0F5C2242F569004A18A5 /* Frameworks */,
     10071                        );
     10072                        buildRules = (
     10073                        );
     10074                        dependencies = (
     10075                        );
     10076                        name = testdfg;
     10077                        productName = testapi;
     10078                        productReference = 52CD0F642242F569004A18A5 /* testdfg */;
     10079                        productType = "com.apple.product-type.tool";
     10080                };
    1004110081                651122F714046A4C002B101D /* testRegExp */ = {
    1004210082                        isa = PBXNativeTarget;
    … …  
    1018510225                                79281BBD20B62B3E002E2A60 /* testmem */,
    1018610226                                5325BDBF21DFF2B100A0DEE1 /* Apply Configuration to XCFileLists */,
     10227                                52CD0F592242F569004A18A5 /* testdfg */,
    1018710228                        );
    1018810229                };
    … …  
    1055010591                        files = (
    1055110592                                14BD689D215191C10050DAFF /* LLIntSettingsExtractor.cpp in Sources */,
     10593                        );
     10594                        runOnlyForDeploymentPostprocessing = 0;
     10595                };
     10596                52CD0F5A2242F569004A18A5 /* Sources */ = {
     10597                        isa = PBXSourcesBuildPhase;
     10598                        buildActionMask = 2147483647;
     10599                        files = (
     10600                                52CD0F682242F71C004A18A5 /* testdfg.cpp in Sources */,
    1055210601                        );
    1055310602                        runOnlyForDeploymentPostprocessing = 0;
    … …  
    1080310852                        targetProxy = 14D9D9D9218462B5009126C2 /* PBXContainerItemProxy */;
    1080410853                };
     10854                52CD0F662242F5A3004A18A5 /* PBXTargetDependency */ = {
     10855                        isa = PBXTargetDependency;
     10856                        target = 52CD0F592242F569004A18A5 /* testdfg */;
     10857                        targetProxy = 52CD0F652242F5A3004A18A5 /* PBXContainerItemProxy */;
     10858                };
    1080510859                53B4BD141F68C2AA00D2BEA3 /* PBXTargetDependency */ = {
    1080610860                        isa = PBXTargetDependency;
    … …  
    1120811262                        name = Production;
    1120911263                };
     11264                52CD0F602242F569004A18A5 /* Debug */ = {
     11265                        isa = XCBuildConfiguration;
     11266                        baseConfigurationReference = BC021BF2136900C300FC5467 /* ToolExecutable.xcconfig */;
     11267                        buildSettings = {
     11268                                PRODUCT_NAME = "$(TARGET_NAME)";
     11269                        };
     11270                        name = Debug;
     11271                };
     11272                52CD0F612242F569004A18A5 /* Release */ = {
     11273                        isa = XCBuildConfiguration;
     11274                        baseConfigurationReference = BC021BF2136900C300FC5467 /* ToolExecutable.xcconfig */;
     11275                        buildSettings = {
     11276                                PRODUCT_NAME = "$(TARGET_NAME)";
     11277                        };
     11278                        name = Release;
     11279                };
     11280                52CD0F622242F569004A18A5 /* Profiling */ = {
     11281                        isa = XCBuildConfiguration;
     11282                        baseConfigurationReference = BC021BF2136900C300FC5467 /* ToolExecutable.xcconfig */;
     11283                        buildSettings = {
     11284                                PRODUCT_NAME = "$(TARGET_NAME)";
     11285                        };
     11286                        name = Profiling;
     11287                };
     11288                52CD0F632242F569004A18A5 /* Production */ = {
     11289                        isa = XCBuildConfiguration;
     11290                        baseConfigurationReference = BC021BF2136900C300FC5467 /* ToolExecutable.xcconfig */;
     11291                        buildSettings = {
     11292                                PRODUCT_NAME = "$(TARGET_NAME)";
     11293                        };
     11294                        name = Production;
     11295                };
    1121011296                5325BDC021DFF2B200A0DEE1 /* Debug */ = {
    1121111297                        isa = XCBuildConfiguration;
    … …  
    1164911735                        defaultConfigurationName = Production;
    1165011736                };
     11737                52CD0F5F2242F569004A18A5 /* Build configuration list for PBXNativeTarget "testdfg" */ = {
     11738                        isa = XCConfigurationList;
     11739                        buildConfigurations = (
     11740                                52CD0F602242F569004A18A5 /* Debug */,
     11741                                52CD0F612242F569004A18A5 /* Release */,
     11742                                52CD0F622242F569004A18A5 /* Profiling */,
     11743                                52CD0F632242F569004A18A5 /* Production */,
     11744                        );
     11745                        defaultConfigurationIsVisible = 0;
     11746                        defaultConfigurationName = Production;
     11747                };
    1165111748                5325BDC421DFF2B200A0DEE1 /* Build configuration list for PBXAggregateTarget "Apply Configuration to XCFileLists" */ = {
    1165211749                        isa = XCConfigurationList;

  • trunk/Source/JavaScriptCore/bytecode/ArrayProfile.h

    r239951 r243278  
    5959const ArrayModes Float64ArrayMode = 1 << 29;
    6060
    61 extern const ArrayModes typedArrayModes[NumberOfTypedArrayTypesExcludingDataView];
     61JS_EXPORT_PRIVATE extern const ArrayModes typedArrayModes[NumberOfTypedArrayTypesExcludingDataView];
    6262
    6363constexpr ArrayModes asArrayModesIgnoringTypedArrays(IndexingType indexingMode)

  • trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h

    r235106 r243278  
    483483SpeculatedType speculationFromStructure(Structure*);
    484484SpeculatedType speculationFromCell(JSCell*);
    485 SpeculatedType speculationFromValue(JSValue);
     485JS_EXPORT_PRIVATE SpeculatedType speculationFromValue(JSValue);
    486486SpeculatedType speculationFromJSType(JSType);
    487487

  • trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.h

    r242990 r243278  
    189189            && !m_value;
    190190    }
     191
     192    bool isBytecodeTop() const
     193    {
     194        return (m_type | SpecBytecodeTop) == m_type
     195            && m_structure.isTop()
     196            && m_arrayModes == ALL_ARRAY_MODES
     197            && !m_value;
     198    }
    191199   
    192200    bool valueIsTop() const
    … …  
    373381    bool validateOSREntryValue(JSValue value, FlushFormat format) const
    374382    {
    375         if (isHeapTop())
     383        if (isBytecodeTop())
    376384            return true;
    377385       
    … …  
    412420    void assertIsRegistered(Graph&) const { }
    413421#else
    414     void checkConsistency() const;
     422    JS_EXPORT_PRIVATE void checkConsistency() const;
    415423    void assertIsRegistered(Graph&) const;
    416424#endif
    … …  
    537545
    538546#if USE(JSVALUE64) && !defined(NDEBUG)
    539     void ensureCanInitializeWithZeros();
     547    JS_EXPORT_PRIVATE void ensureCanInitializeWithZeros();
    540548#endif
    541549   

  • trunk/Source/JavaScriptCore/dfg/DFGJITCompiler.cpp

    r241222 r243278  
    585585        Node* node = basicBlock.variablesAtHead.argument(argument);
    586586        if (!node || !node->shouldGenerate())
    587             entry->m_expectedValues.argument(argument).makeHeapTop();
     587            entry->m_expectedValues.argument(argument).makeBytecodeTop();
    588588    }
    589589    for (size_t local = 0; local < basicBlock.variablesAtHead.numberOfLocals(); ++local) {
    590590        Node* node = basicBlock.variablesAtHead.local(local);
    591591        if (!node || !node->shouldGenerate())
    592             entry->m_expectedValues.local(local).makeHeapTop();
     592            entry->m_expectedValues.local(local).makeBytecodeTop();
    593593        else {
    594594            VariableAccessData* variable = node->variableAccessData();

  • trunk/Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h

    r228500 r243278  
    233233
    234234    bool contains(RegisteredStructure) const;
    235     bool contains(Structure* structure) const;
     235    JS_EXPORT_PRIVATE bool contains(Structure* structure) const;
    236236   
    237237    bool isSubsetOf(const RegisteredStructureSet& other) const;

  • trunk/Source/JavaScriptCore/shell/CMakeLists.txt

    r243005 r243278  
    6969)
    7070
     71set(TESTDFG_SOURCES
     72    ../dfg/testdfg.cpp
     73)
     74
    7175if (DEVELOPER_MODE AND NOT WIN32)
    7276    add_executable(testmasm ${TESTMASM_SOURCES})
    … …  
    7882    add_executable(testair ${TESTAIR_SOURCES})
    7983    target_link_libraries(testair ${JSC_LIBRARIES})
     84
     85    add_executable(testdfg ${TESTDFG_SOURCES})
     86    target_link_libraries(testdfg ${JSC_LIBRARIES})
    8087
    8188    add_executable(testapi ${TESTAPI_SOURCES})

  • trunk/Tools/ChangeLog

    r243275 r243278  
     12019-03-20  Saam Barati  <sbarati@apple.com>
     2
     3        DFG::AbstractValue::validateOSREntry is wrong when isHeapTop and the incoming value is Empty
     4        https://bugs.webkit.org/show_bug.cgi?id=195721
     5
     6        Reviewed by Filip Pizlo.
     7
     8        * Scripts/run-javascriptcore-tests:
     9
    1102019-03-20  Simon Fraser  <simon.fraser@apple.com>
    211

  • trunk/Tools/Scripts/run-javascriptcore-tests

    r237130 r243278  
    7272my $runTestAir = RUN_IF_NO_TESTS_SPECIFIED;
    7373my $runTestB3 = RUN_IF_NO_TESTS_SPECIFIED;
     74my $runTestDFG = RUN_IF_NO_TESTS_SPECIFIED;
    7475my $runTestAPI = RUN_IF_NO_TESTS_SPECIFIED;
    7576my $runJSCStress = RUN_IF_NO_TESTS_SPECIFIED;
    … …  
    129130}
    130131
     132if ($ENV{RUN_JAVASCRIPTCORE_TESTS_TESTDFG}) {
     133    if ($ENV{RUN_JAVASCRIPTCORE_TESTS_TESTDFG} eq "true") {
     134        $runTestDFG = ENV_VAR_SAYS_DO_RUN;
     135    } elsif ($ENV{RUN_JAVASCRIPTCORE_TESTS_TESTDFG} eq "false") {
     136        $runTestDFG = ENV_VAR_SAYS_DONT_RUN;
     137    } else {
     138        print "Don't recognize value for RUN_JAVASCRIPTCORE_TESTS_TESTDFG environment variable: '"
     139            . $ENV{RUN_JAVASCRIPTCORE_TESTS_TESTDFG} . "'. Should be set to 'true' or 'false'.\n";
     140    }
     141}
     142
    131143if ($ENV{RUN_JAVASCRIPTCORE_TESTS_TESTAPI}) {
    132144    if ($ENV{RUN_JAVASCRIPTCORE_TESTS_TESTAPI} eq "true") {
    … …  
    182194my $testairDefault = defaultStringForTestState($runTestAir);
    183195my $testb3Default = defaultStringForTestState($runTestB3);
     196my $testDFGDefault = defaultStringForTestState($runTestDFG);
    184197my $testapiDefault = defaultStringForTestState($runTestAPI);
    185198my $jscStressDefault = defaultStringForTestState($runJSCStress);
    … …  
    199212  --[no-]testair                Only run (or don't run) testair (default: $testairDefault)
    200213  --[no-]testb3                 Only run (or don't run) testb3 (default: $testb3Default)
     214  --[no-]testdfg                Only run (or don't run) testdfg (default: $testDFGDefault)
    201215  --[no-]testapi                Only run (or don't run) testapi (default: $testapiDefault)
    202216  --[no-]jsc-stress             Only run (or don't run) the JSC stress tests (default: $jscStressDefault)
    … …  
    239253  - set RUN_JAVASCRIPTCORE_TESTS_TESTAIR to "true" or "false" (no quotes) to determine if we run testair by default.
    240254  - set RUN_JAVASCRIPTCORE_TESTS_TESTB3 to "true" or "false" (no quotes) to determine if we run testb3 by default.
     255  - set RUN_JAVASCRIPTCORE_TESTS_TESTDFG to "true" or "false" (no quotes) to determine if we run testdfg by default.
    241256  - set RUN_JAVASCRIPTCORE_TESTS_TESTAPI to "true" or "false" (no quotes) to determine if we run testapi by default.
    242257  - set RUN_JAVASCRIPTCORE_TESTS_BUILD to "true" or "false" (no quotes) to set the should-we-build-before-running-tests setting.
    … …  
    258273    'testair!' => \$runTestAir,
    259274    'testb3!' => \$runTestB3,
     275    'testdfg!' => \$runTestDFG,
    260276    'testapi!' => \$runTestAPI,
    261277    'jsc-stress!' => \$runJSCStress,
    … …  
    288304   || $runTestAir == DO_RUN
    289305   || $runTestB3 == DO_RUN
     306   || $runTestDFG == DO_RUN
    290307   || $runTestAPI == DO_RUN
    291308   || $runJSCStress == DO_RUN
    … …  
    307324$runTestAir = enableTestOrNot($runTestAir);
    308325$runTestB3 = enableTestOrNot($runTestB3);
     326$runTestDFG = enableTestOrNot($runTestDFG);
    309327$runTestAPI = enableTestOrNot($runTestAPI);
    310328$runJSCStress = enableTestOrNot($runJSCStress);
    … …  
    395413if ($runTestAir) { runTest("testair", "allAirTestsPassed") }
    396414if ($runTestB3) { runTest("testb3", "allB3TestsPassed") }
     415if ($runTestDFG) { runTest("testdfg", "allDFGTestsPassed") }
    397416if ($runTestAPI) { runTest("testapi", "allApiTestsPassed") }
    398417
Note: See TracChangeset for help on using the changeset viewer.