Disable function.arguments
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Sep 2014 20:27:59 +0000 (20:27 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Sep 2014 20:27:59 +0000 (20:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137167

Source/JavaScriptCore:

Rubber stamped by Geoffrey Garen.

Add an option to disable function.arguments. Add a test for disabling it.

Disabling function.arguments means that it returns an Arguments object that claims that
there were zero arguments. All other Arguments functionality still works, so any code
that tries to inspect this object will still think that it is looking at a perfectly
valid Arguments object.

This also makes function.arguments disabled by default. Note that the RJST harness will
enable them by default, to continue to get test coverage for the code that implements
the feature.

We will rip out that code once we're confident that it's really safe to remove this
feature. Only once we rip out that support will we be able to do optimizations to
leverage the lack of this feature. It's important to keep the support code, and the test
infrastructure, in place before we are confident. The logic to keep this working touches
the entire compiler and a large chunk of the runtime, so reimplementing it - or even
merging it back in - would be a nightmare. That's also basically the reason why we want
to rip it out if at all possible. It's a lot of terrible code.

* interpreter/StackVisitor.cpp:
(JSC::StackVisitor::Frame::createArguments):
* runtime/Arguments.h:
(JSC::Arguments::create):
(JSC::Arguments::finishCreation):
* runtime/Options.h:
* tests/stress/disable-function-dot-arguments.js: Added.
(foo):
(bar):

Tools:

Rubber stamped by Geoffrey Garen

Enable the feature by default during tests.

* Scripts/run-jsc-stress-tests:

LayoutTests:

Rubber stamped by Geoffrey Garen.

Don't remove the tests for this, yet - but mark them as failing. We will rebase these,
or remove them entirely, once we know that it's safe to rip out this feature entirely.

* TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@174036 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/interpreter/StackVisitor.cpp
Source/JavaScriptCore/runtime/Arguments.h
Source/JavaScriptCore/runtime/Options.h
Source/JavaScriptCore/tests/stress/disable-function-dot-arguments.js [new file with mode: 0644]
Tools/ChangeLog
Tools/Scripts/run-jsc-stress-tests

index fd6b397d28544c6ec91d442864ad6ea35febd4b5..7fa3bf0510f00fbecc734f4a5d8ccb8f8170551a 100644 (file)
@@ -1,3 +1,15 @@
+2014-09-26  Filip Pizlo  <fpizlo@apple.com>
+
+        Disable function.arguments
+        https://bugs.webkit.org/show_bug.cgi?id=137167
+
+        Rubber stamped by Geoffrey Garen.
+        
+        Don't remove the tests for this, yet - but mark them as failing. We will rebase these,
+        or remove them entirely, once we know that it's safe to rip out this feature entirely.
+
+        * TestExpectations:
+
 2014-09-27  Benjamin Poulain  <bpoulain@apple.com>
 
         Chaining multiple :nth-child() does not work properly
index 783628c4747b7fbfe22471a57d5d6127c9ed176d..7e7276152e220fe8ad8ebb89134665b447b0b22c 100644 (file)
@@ -199,3 +199,27 @@ webkit.org/b/136754 css3/flexbox/csswg/ttwf-reftest-flex-wrap.html [ ImageOnlyFa
 
 # nth-child tests takes long time and Debug build sometimes timeouts because there are many test cases.
 webkit.org/b/137149 fast/selectors/nth-child-of-basics.html [ Slow ]
+
+# Deprecated tests for function.arguments. These tests need to stay in the tree because the RJST
+# harness will run these after forcibly reenabling the function.arguments feature. We want to
+# keep testing the feature for as long as we haven't achieved confidence that it is safe to
+# remove it entirely.
+webkit.org/b/137167 js/dfg-arguments-unexpected-escape.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-become-double.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-become-int32.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-int32.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-osr-exit-and-capture.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-reset-changetype.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-reset.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-simple.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-use-directly-from-inlined-code.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-use-from-all-the-places.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-use-from-getter.html [ Failure ]
+webkit.org/b/137167 js/dfg-inline-arguments-use-from-uninlined-code.html [ Failure ]
+webkit.org/b/137167 js/dfg-tear-off-arguments-not-activation.html [ Failure ]
+webkit.org/b/137167 js/dfg-tear-off-function-dot-arguments.html [ Failure ]
+webkit.org/b/137167 js/dom/exception-thrown-from-function-with-lazy-activation.html [ Failure ]
+webkit.org/b/137167 js/dom/function-dot-arguments2.html [ Failure ]
+webkit.org/b/137167 js/function-dot-arguments.html [ Failure ]
+webkit.org/b/137167 js/kde/function.html [ Failure ]
+webkit.org/b/137167 js/kde/function_arguments.html [ Failure ]
index d90b6ea1d6c2faf9959279da756efb0370a89f19..a715868bdff93e3fbdf6db90800d781f752b1d0f 100644 (file)
@@ -1,3 +1,39 @@
+2014-09-26  Filip Pizlo  <fpizlo@apple.com>
+
+        Disable function.arguments
+        https://bugs.webkit.org/show_bug.cgi?id=137167
+
+        Rubber stamped by Geoffrey Garen.
+        
+        Add an option to disable function.arguments. Add a test for disabling it.
+        
+        Disabling function.arguments means that it returns an Arguments object that claims that
+        there were zero arguments. All other Arguments functionality still works, so any code
+        that tries to inspect this object will still think that it is looking at a perfectly
+        valid Arguments object.
+        
+        This also makes function.arguments disabled by default. Note that the RJST harness will
+        enable them by default, to continue to get test coverage for the code that implements
+        the feature.
+        
+        We will rip out that code once we're confident that it's really safe to remove this
+        feature. Only once we rip out that support will we be able to do optimizations to
+        leverage the lack of this feature. It's important to keep the support code, and the test
+        infrastructure, in place before we are confident. The logic to keep this working touches
+        the entire compiler and a large chunk of the runtime, so reimplementing it - or even
+        merging it back in - would be a nightmare. That's also basically the reason why we want
+        to rip it out if at all possible. It's a lot of terrible code.
+
+        * interpreter/StackVisitor.cpp:
+        (JSC::StackVisitor::Frame::createArguments):
+        * runtime/Arguments.h:
+        (JSC::Arguments::create):
+        (JSC::Arguments::finishCreation):
+        * runtime/Options.h:
+        * tests/stress/disable-function-dot-arguments.js: Added.
+        (foo):
+        (bar):
+
 2014-09-26  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Automatic Inspection should continue once all breakpoints are loaded
index 995ab6b830bb7a9a39390150d77de954ceedd92f..29dc967f484ddeef11faaa585aebcedcd26bd5bb 100644 (file)
@@ -261,15 +261,20 @@ Arguments* StackVisitor::Frame::createArguments()
     CallFrame* physicalFrame = m_callFrame;
     VM& vm = physicalFrame->vm();
     Arguments* arguments;
+    ArgumentsMode mode;
+    if (Options::enableFunctionDotArguments())
+        mode = NormalArgumentsCreationMode;
+    else
+        mode = FakeArgumentValuesCreationMode;
 #if ENABLE(DFG_JIT)
     if (isInlinedFrame()) {
         ASSERT(m_inlineCallFrame);
-        arguments = Arguments::create(vm, physicalFrame, m_inlineCallFrame);
+        arguments = Arguments::create(vm, physicalFrame, m_inlineCallFrame, mode);
         arguments->tearOff(physicalFrame, m_inlineCallFrame);
     } else 
 #endif
     {
-        arguments = Arguments::create(vm, physicalFrame);
+        arguments = Arguments::create(vm, physicalFrame, mode);
         arguments->tearOff(physicalFrame);
     }
     return arguments;
index 2d4ada5677ccb3c152e3b2c0a12551aa3f52b3cb..7c8e857f387d91373c620cb39fe9131a7abba027 100644 (file)
 
 namespace JSC {
 
+enum ArgumentsMode {
+    NormalArgumentsCreationMode,
+    FakeArgumentValuesCreationMode
+};
+
 class Arguments : public JSNonFinalObject {
     friend class JIT;
     friend class JSArgumentsIterator;
 public:
     typedef JSNonFinalObject Base;
 
-    static Arguments* create(VM& vm, CallFrame* callFrame)
+    static Arguments* create(VM& vm, CallFrame* callFrame, ArgumentsMode mode = NormalArgumentsCreationMode)
     {
         Arguments* arguments = new (NotNull, allocateCell<Arguments>(vm.heap)) Arguments(callFrame);
-        arguments->finishCreation(callFrame);
+        arguments->finishCreation(callFrame, mode);
         return arguments;
     }
         
-    static Arguments* create(VM& vm, CallFrame* callFrame, InlineCallFrame* inlineCallFrame)
+    static Arguments* create(VM& vm, CallFrame* callFrame, InlineCallFrame* inlineCallFrame, ArgumentsMode mode = NormalArgumentsCreationMode)
     {
         Arguments* arguments = new (NotNull, allocateCell<Arguments>(vm.heap)) Arguments(callFrame);
-        arguments->finishCreation(callFrame, inlineCallFrame);
+        arguments->finishCreation(callFrame, inlineCallFrame, mode);
         return arguments;
     }
 
@@ -107,8 +112,8 @@ public:
 protected:
     static const unsigned StructureFlags = OverridesGetOwnPropertySlot | InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | OverridesGetPropertyNames | JSObject::StructureFlags;
 
-    void finishCreation(CallFrame*);
-    void finishCreation(CallFrame*, InlineCallFrame*);
+    void finishCreation(CallFrame*, ArgumentsMode);
+    void finishCreation(CallFrame*, InlineCallFrame*, ArgumentsMode);
 
 private:
     static bool getOwnPropertySlot(JSObject*, ExecState*, PropertyName, PropertySlot&);
@@ -271,62 +276,88 @@ inline WriteBarrierBase<Unknown>& Arguments::argument(size_t argument)
     return m_lexicalEnvironment->registerAt(index - m_slowArgumentData->bytecodeToMachineCaptureOffset());
 }
 
-inline void Arguments::finishCreation(CallFrame* callFrame)
+inline void Arguments::finishCreation(CallFrame* callFrame, ArgumentsMode mode)
 {
     Base::finishCreation(callFrame->vm());
     ASSERT(inherits(info()));
 
     JSFunction* callee = jsCast<JSFunction*>(callFrame->callee());
-    m_numArguments = callFrame->argumentCount();
-    m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers());
     m_callee.set(callFrame->vm(), this, callee);
     m_overrodeLength = false;
     m_overrodeCallee = false;
     m_overrodeCaller = false;
     m_isStrictMode = callFrame->codeBlock()->isStrictMode();
 
-    CodeBlock* codeBlock = callFrame->codeBlock();
-    if (codeBlock->hasSlowArguments()) {
-        SymbolTable* symbolTable = codeBlock->symbolTable();
-        const SlowArgument* slowArguments = codeBlock->machineSlowArguments();
-        allocateSlowArguments(callFrame->vm());
-        size_t count = std::min<unsigned>(m_numArguments, symbolTable->parameterCount());
-        for (size_t i = 0; i < count; ++i)
-            m_slowArgumentData->slowArguments()[i] = slowArguments[i];
-        m_slowArgumentData->setBytecodeToMachineCaptureOffset(
-            codeBlock->framePointerOffsetToGetActivationRegisters());
-    }
+    switch (mode) {
+    case NormalArgumentsCreationMode: {
+        m_numArguments = callFrame->argumentCount();
+        m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers());
+
+        CodeBlock* codeBlock = callFrame->codeBlock();
+        if (codeBlock->hasSlowArguments()) {
+            SymbolTable* symbolTable = codeBlock->symbolTable();
+            const SlowArgument* slowArguments = codeBlock->machineSlowArguments();
+            allocateSlowArguments(callFrame->vm());
+            size_t count = std::min<unsigned>(m_numArguments, symbolTable->parameterCount());
+            for (size_t i = 0; i < count; ++i)
+                m_slowArgumentData->slowArguments()[i] = slowArguments[i];
+            m_slowArgumentData->setBytecodeToMachineCaptureOffset(
+                codeBlock->framePointerOffsetToGetActivationRegisters());
+        }
 
-    // The bytecode generator omits op_tear_off_lexical_environment in cases of no
-    // declared parameters, so we need to tear off immediately.
-    if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
+        // The bytecode generator omits op_tear_off_lexical_environment in cases of no
+        // declared parameters, so we need to tear off immediately.
+        if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
+            tearOff(callFrame);
+        break;
+    }
+        
+    case FakeArgumentValuesCreationMode: {
+        m_numArguments = 0;
+        m_registers = nullptr;
         tearOff(callFrame);
+        break;
+    } }
+        
 }
 
-inline void Arguments::finishCreation(CallFrame* callFrame, InlineCallFrame* inlineCallFrame)
+inline void Arguments::finishCreation(CallFrame* callFrame, InlineCallFrame* inlineCallFrame, ArgumentsMode mode)
 {
     Base::finishCreation(callFrame->vm());
     ASSERT(inherits(info()));
 
     JSFunction* callee = inlineCallFrame->calleeForCallFrame(callFrame);
-    m_numArguments = inlineCallFrame->arguments.size() - 1;
-    
-    if (m_numArguments) {
-        int offsetForArgumentOne = inlineCallFrame->arguments[1].virtualRegister().offset();
-        m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers()) + offsetForArgumentOne - virtualRegisterForArgument(1).offset();
-    } else
-        m_registers = 0;
     m_callee.set(callFrame->vm(), this, callee);
     m_overrodeLength = false;
     m_overrodeCallee = false;
     m_overrodeCaller = false;
     m_isStrictMode = jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->isStrictMode();
-    ASSERT(!jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->symbolTable(inlineCallFrame->specializationKind())->slowArguments());
 
-    // The bytecode generator omits op_tear_off_lexical_environment in cases of no
-    // declared parameters, so we need to tear off immediately.
-    if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
-        tearOff(callFrame, inlineCallFrame);
+    switch (mode) {
+    case NormalArgumentsCreationMode: {
+        m_numArguments = inlineCallFrame->arguments.size() - 1;
+        
+        if (m_numArguments) {
+            int offsetForArgumentOne = inlineCallFrame->arguments[1].virtualRegister().offset();
+            m_registers = reinterpret_cast<WriteBarrierBase<Unknown>*>(callFrame->registers()) + offsetForArgumentOne - virtualRegisterForArgument(1).offset();
+        } else
+            m_registers = 0;
+        
+        ASSERT(!jsCast<FunctionExecutable*>(inlineCallFrame->executable.get())->symbolTable(inlineCallFrame->specializationKind())->slowArguments());
+        
+        // The bytecode generator omits op_tear_off_lexical_environment in cases of no
+        // declared parameters, so we need to tear off immediately.
+        if (m_isStrictMode || !callee->jsExecutable()->parameterCount())
+            tearOff(callFrame, inlineCallFrame);
+        break;
+    }
+        
+    case FakeArgumentValuesCreationMode: {
+        m_numArguments = 0;
+        m_registers = nullptr;
+        tearOff(callFrame);
+        break;
+    } }
 }
 
 } // namespace JSC
index 020795d31685e1901c6f59c6942b71940e0c7d7c..9e7eb5230de6ce83a9471d1e862fde206efe609e 100644 (file)
@@ -112,6 +112,8 @@ typedef const char* optionString;
     v(bool, forceDebuggerBytecodeGeneration, false) \
     v(bool, forceProfilerBytecodeGeneration, false) \
     \
+    v(bool, enableFunctionDotArguments, false) \
+    \
     /* showDisassembly implies showDFGDisassembly. */ \
     v(bool, showDisassembly, false) \
     v(bool, showDFGDisassembly, false) \
diff --git a/Source/JavaScriptCore/tests/stress/disable-function-dot-arguments.js b/Source/JavaScriptCore/tests/stress/disable-function-dot-arguments.js
new file mode 100644 (file)
index 0000000..ba94fc3
--- /dev/null
@@ -0,0 +1,20 @@
+//@ run("function-dot-arguments", "--enableFunctionDotArguments=false")
+
+function foo() {
+    var a = bar.arguments;
+    if (a.length != 0)
+        throw "Error: arguments have non-zero length";
+    for (var i = 0; i < 100; ++i) {
+        if (a[i] !== void 0)
+            throw "Error: argument " + i + " has non-undefined value";
+    }
+}
+
+function bar() {
+    foo();
+}
+
+bar();
+bar(1);
+bar(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+
index 3f2e899f49d13b89e38481f982975e40ea4c9960..197750db65d7961e1c7e0ed19dddb4889d0c549f 100644 (file)
@@ -1,3 +1,14 @@
+2014-09-26  Filip Pizlo  <fpizlo@apple.com>
+
+        Disable function.arguments
+        https://bugs.webkit.org/show_bug.cgi?id=137167
+
+        Rubber stamped by Geoffrey Garen
+        
+        Enable the feature by default during tests.
+
+        * Scripts/run-jsc-stress-tests:
+
 2014-09-26  Beth Dakin  <bdakin@apple.com>
 
         Many platform/mac-wk2/tiled-drawing/ tests fail when run on a retina device
index 55f6a8ac12c7c7fa6d54e1a6aa3997325855efa4..a0314f3a813c79f8e486e4aa383d8d760d280ee1 100755 (executable)
@@ -260,9 +260,9 @@ $hostOS = determineOS
 
 $numFailures = 0
 
+BASE_OPTIONS = ["--useFTLJIT=false", "--enableFunctionDotArguments=true"]
 EAGER_OPTIONS = ["--thresholdForJITAfterWarmUp=10", "--thresholdForJITSoon=10", "--thresholdForOptimizeAfterWarmUp=20", "--thresholdForOptimizeAfterLongWarmUp=20", "--thresholdForOptimizeSoon=20", "--thresholdForFTLOptimizeAfterWarmUp=20", "--thresholdForFTLOptimizeSoon=20"]
 NO_CJIT_OPTIONS = ["--enableConcurrentJIT=false", "--thresholdForJITAfterWarmUp=100"]
-NO_FTL_OPTIONS = ["--useFTLJIT=false"]
 FTL_OPTIONS = ["--useFTLJIT=true", "--enableExperimentalFTLCoverage=true"]
 
 $runlist = []
@@ -569,7 +569,7 @@ def slow!
 end
 
 def run(kind, *options)
-    addRunCommand(kind, [pathToVM.to_s] + NO_FTL_OPTIONS + options + [$benchmark.to_s], silentOutputHandler, simpleErrorHandler)
+    addRunCommand(kind, [pathToVM.to_s] + BASE_OPTIONS + options + [$benchmark.to_s], silentOutputHandler, simpleErrorHandler)
 end
 
 def runDefault
@@ -728,7 +728,7 @@ def runLayoutTest(kind, *options)
     prepareExtraRelativeFiles(["../#{testName}-expected.txt"], $benchmarkDirectory)
     prepareExtraAbsoluteFiles(LAYOUTTESTS_PATH, ["resources/standalone-pre.js", "resources/standalone-post.js"])
 
-    args = [pathToVM.to_s] + NO_FTL_OPTIONS + options +
+    args = [pathToVM.to_s] + BASE_OPTIONS + options +
         [(Pathname.new("resources") + "standalone-pre.js").to_s,
          $benchmark.to_s,
          (Pathname.new("resources") + "standalone-post.js").to_s]
@@ -810,7 +810,7 @@ def runMozillaTest(kind, mode, extraFiles, *options)
         kind = "mozilla"
     end
     prepareExtraRelativeFiles(extraFiles.map{|v| (Pathname("..") + v).to_s}, $collection)
-    args = [pathToVM.to_s] + NO_FTL_OPTIONS + options + extraFiles.map{|v| v.to_s} + [$benchmark.to_s]
+    args = [pathToVM.to_s] + BASE_OPTIONS + options + extraFiles.map{|v| v.to_s} + [$benchmark.to_s]
     case mode
     when :normal
         errorHandler = mozillaErrorHandler