Title: [201830] trunk/Source/_javascript_Core
Revision
201830
Author
mark....@apple.com
Date
2016-06-08 13:59:49 -0700 (Wed, 08 Jun 2016)

Log Message

Simplify Interpreter::StackFrame.
https://bugs.webkit.org/show_bug.cgi?id=158498

Reviewed by Saam Barati.

Previously, Interpreter::StackFrame (which is used to capture info for
Error.stack) eagerly extracts info out of CodeBlock and duplicates the work that
CodeBlock does to compute line and column numbers (amongst other things).

This patch does away with the eager extraction and only stashes the CodeBlock
pointer in the Interpreter::StackFrame.  Instead, Interpreter::StackFrame will
provide methods for computing the desired values on request later.

One difference in implementation: the old StackFrame offers a sourceURL and a
friendlySourceURL().  The only difference between the 2 is that for native
functions, sourceURL returns an empty string, and friendlySourceURL() returns
"[native code]".  This is how it affects the clients of StackFrame:

    - In the old code, the Error object's addErrorInfoAndGetBytecodeOffset() and
      the inspector's createScriptCallStackFromException() would check if
      sourceURL is empty.  If so, they will use this as an indicator to use
      alternate source info in the Error object e.g. url and line numbers from
      the parser that produced a SyntaxError.

    - In the new implementation, StackFrame only has a sourceURL() function that
      behaves like the old friendlySourceURL().  The client code which were
      relying on sourceURL being empty, will now explicitly check if the
      StackFrame is for native code using a new isNative() query in addition to
      the sourceURL being empty.  This achieve functional parity with the old
      behavior.

Also fix Error.cpp's addErrorInfoAndGetBytecodeOffset() to take a bytecodeOffset
pointer instead of a reference.  The bytecodeOffset arg is supposed to be
optional, but was implemented in a unclear way.  This change clarifies it.

* inspector/ScriptCallStackFactory.cpp:
(Inspector::createScriptCallStackFromException):
* interpreter/Interpreter.cpp:
(JSC::StackFrame::sourceID):
(JSC::StackFrame::sourceURL):
(JSC::StackFrame::functionName):
(JSC::eval):
(JSC::Interpreter::isOpcode):
(JSC::StackFrame::computeLineAndColumn):
(JSC::StackFrame::toString):
(JSC::GetStackTraceFunctor::operator()):
(JSC::StackFrame::friendlySourceURL): Deleted.
(JSC::StackFrame::friendlyFunctionName): Deleted.
(JSC::getStackFrameCodeType): Deleted.
(JSC::StackFrame::expressionInfo): Deleted.
* interpreter/Interpreter.h:
(JSC::StackFrame::isNative):
* runtime/Error.cpp:
(JSC::addErrorInfoAndGetBytecodeOffset):
(JSC::addErrorInfo):
* runtime/Error.h:
* runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::finishCreation):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (201829 => 201830)


--- trunk/Source/_javascript_Core/ChangeLog	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-06-08 20:59:49 UTC (rev 201830)
@@ -1,3 +1,64 @@
+2016-06-08  Mark Lam  <mark....@apple.com>
+
+        Simplify Interpreter::StackFrame.
+        https://bugs.webkit.org/show_bug.cgi?id=158498
+
+        Reviewed by Saam Barati.
+
+        Previously, Interpreter::StackFrame (which is used to capture info for
+        Error.stack) eagerly extracts info out of CodeBlock and duplicates the work that
+        CodeBlock does to compute line and column numbers (amongst other things).
+
+        This patch does away with the eager extraction and only stashes the CodeBlock
+        pointer in the Interpreter::StackFrame.  Instead, Interpreter::StackFrame will
+        provide methods for computing the desired values on request later.
+
+        One difference in implementation: the old StackFrame offers a sourceURL and a
+        friendlySourceURL().  The only difference between the 2 is that for native
+        functions, sourceURL returns an empty string, and friendlySourceURL() returns
+        "[native code]".  This is how it affects the clients of StackFrame:
+
+            - In the old code, the Error object's addErrorInfoAndGetBytecodeOffset() and
+              the inspector's createScriptCallStackFromException() would check if
+              sourceURL is empty.  If so, they will use this as an indicator to use
+              alternate source info in the Error object e.g. url and line numbers from
+              the parser that produced a SyntaxError.
+
+            - In the new implementation, StackFrame only has a sourceURL() function that
+              behaves like the old friendlySourceURL().  The client code which were
+              relying on sourceURL being empty, will now explicitly check if the
+              StackFrame is for native code using a new isNative() query in addition to
+              the sourceURL being empty.  This achieve functional parity with the old
+              behavior.
+
+        Also fix Error.cpp's addErrorInfoAndGetBytecodeOffset() to take a bytecodeOffset
+        pointer instead of a reference.  The bytecodeOffset arg is supposed to be
+        optional, but was implemented in a unclear way.  This change clarifies it.
+
+        * inspector/ScriptCallStackFactory.cpp:
+        (Inspector::createScriptCallStackFromException):
+        * interpreter/Interpreter.cpp:
+        (JSC::StackFrame::sourceID):
+        (JSC::StackFrame::sourceURL):
+        (JSC::StackFrame::functionName):
+        (JSC::eval):
+        (JSC::Interpreter::isOpcode):
+        (JSC::StackFrame::computeLineAndColumn):
+        (JSC::StackFrame::toString):
+        (JSC::GetStackTraceFunctor::operator()):
+        (JSC::StackFrame::friendlySourceURL): Deleted.
+        (JSC::StackFrame::friendlyFunctionName): Deleted.
+        (JSC::getStackFrameCodeType): Deleted.
+        (JSC::StackFrame::expressionInfo): Deleted.
+        * interpreter/Interpreter.h:
+        (JSC::StackFrame::isNative):
+        * runtime/Error.cpp:
+        (JSC::addErrorInfoAndGetBytecodeOffset):
+        (JSC::addErrorInfo):
+        * runtime/Error.h:
+        * runtime/ErrorInstance.cpp:
+        (JSC::ErrorInstance::finishCreation):
+
 2016-06-08  Keith Miller  <keith_mil...@apple.com>
 
         We should be able to lookup symbols by identifier in builtins

Modified: trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp (201829 => 201830)


--- trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/inspector/ScriptCallStackFactory.cpp	2016-06-08 20:59:49 UTC (rev 201830)
@@ -143,8 +143,8 @@
         unsigned line;
         unsigned column;
         stackTrace[i].computeLineAndColumn(line, column);
-        String functionName = stackTrace[i].friendlyFunctionName(vm);
-        frames.append(ScriptCallFrame(functionName, stackTrace[i].friendlySourceURL(), static_cast<SourceID>(stackTrace[i].sourceID), line, column));
+        String functionName = stackTrace[i].functionName(vm);
+        frames.append(ScriptCallFrame(functionName, stackTrace[i].sourceURL(), static_cast<SourceID>(stackTrace[i].sourceID()), line, column));
     }
 
     // Fallback to getting at least the line and sourceURL from the exception object if it has values and the exceptionStack doesn't.
@@ -158,10 +158,10 @@
             extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL);
             frames.append(ScriptCallFrame(String(), exceptionSourceURL, noSourceID, lineNumber, columnNumber));
         } else {
-            if (stackTrace[0].sourceURL.isEmpty()) {
+            if (stackTrace[0].isNative() || stackTrace[0].sourceURL().isEmpty()) {
                 const ScriptCallFrame& firstCallFrame = frames.first();
                 extractSourceInformationFromException(exec, exceptionObject, &lineNumber, &columnNumber, &exceptionSourceURL);
-                frames[0] = ScriptCallFrame(firstCallFrame.functionName(), exceptionSourceURL, stackTrace[0].sourceID, lineNumber, columnNumber);
+                frames[0] = ScriptCallFrame(firstCallFrame.functionName(), exceptionSourceURL, stackTrace[0].sourceID(), lineNumber, columnNumber);
             }
         }
     }

Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.cpp (201829 => 201830)


--- trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.cpp	2016-06-08 20:59:49 UTC (rev 201830)
@@ -87,49 +87,44 @@
 
 namespace JSC {
 
-String StackFrame::friendlySourceURL() const
+intptr_t StackFrame::sourceID() const
 {
-    String traceLine;
-    
-    switch (codeType) {
-    case StackFrameEvalCode:
-    case StackFrameModuleCode:
-    case StackFrameFunctionCode:
-    case StackFrameGlobalCode:
-        if (!sourceURL.isEmpty())
-            traceLine = sourceURL.impl();
-        break;
-    case StackFrameNativeCode:
-        traceLine = "[native code]";
-        break;
-    }
-    return traceLine.isNull() ? emptyString() : traceLine;
+    if (!codeBlock)
+        return noSourceID;
+    return codeBlock->ownerScriptExecutable()->sourceID();
 }
 
-String StackFrame::friendlyFunctionName(VM& vm) const
+String StackFrame::sourceURL() const
 {
-    String traceLine;
-    JSObject* stackFrameCallee = callee.get();
+    if (!codeBlock)
+        return ASCIILiteral("[native code]");
 
-    switch (codeType) {
-    case StackFrameEvalCode:
-        traceLine = "eval code";
-        break;
-    case StackFrameModuleCode:
-        traceLine = "module code";
-        break;
-    case StackFrameNativeCode:
-        if (callee)
-            traceLine = getCalculatedDisplayName(vm, stackFrameCallee).impl();
-        break;
-    case StackFrameFunctionCode:
-        traceLine = getCalculatedDisplayName(vm, stackFrameCallee).impl();
-        break;
-    case StackFrameGlobalCode:
-        traceLine = "global code";
-        break;
+    String sourceURL = codeBlock->ownerScriptExecutable()->sourceURL();
+    if (!sourceURL.isNull())
+        return sourceURL;
+    return emptyString();
+}
+
+String StackFrame::functionName(VM& vm) const
+{
+    if (codeBlock) {
+        switch (codeBlock->codeType()) {
+        case EvalCode:
+            return ASCIILiteral("eval code");
+        case ModuleCode:
+            return ASCIILiteral("module code");
+        case FunctionCode:
+            break;
+        case GlobalCode:
+            return ASCIILiteral("global code");
+        default:
+            ASSERT_NOT_REACHED();
+        }
     }
-    return traceLine.isNull() ? emptyString() : traceLine;
+    String name;
+    if (callee)
+        name = getCalculatedDisplayName(vm, callee.get()).impl();
+    return name.isNull() ? emptyString() : name;
 }
 
 JSValue eval(CallFrame* callFrame)
@@ -449,25 +444,6 @@
 #endif
 }
 
-static StackFrameCodeType getStackFrameCodeType(StackVisitor& visitor)
-{
-    switch (visitor->codeType()) {
-    case StackVisitor::Frame::Eval:
-        return StackFrameEvalCode;
-    case StackVisitor::Frame::Module:
-        return StackFrameModuleCode;
-    case StackVisitor::Frame::Function:
-        return StackFrameFunctionCode;
-    case StackVisitor::Frame::Global:
-        return StackFrameGlobalCode;
-    case StackVisitor::Frame::Native:
-        ASSERT_NOT_REACHED();
-        return StackFrameNativeCode;
-    }
-    RELEASE_ASSERT_NOT_REACHED();
-    return StackFrameGlobalCode;
-}
-
 void StackFrame::computeLineAndColumn(unsigned& line, unsigned& column)
 {
     if (!codeBlock) {
@@ -479,34 +455,24 @@
     int divot = 0;
     int unusedStartOffset = 0;
     int unusedEndOffset = 0;
-    unsigned divotLine = 0;
-    unsigned divotColumn = 0;
-    expressionInfo(divot, unusedStartOffset, unusedEndOffset, divotLine, divotColumn);
+    codeBlock->expressionRangeForBytecodeOffset(bytecodeOffset, divot, unusedStartOffset, unusedEndOffset, line, column);
 
-    line = divotLine + lineOffset;
-    column = divotColumn + (divotLine ? 1 : firstLineColumnOffset);
-
+    ScriptExecutable* executable = codeBlock->ownerScriptExecutable();
     if (executable->hasOverrideLineNumber())
         line = executable->overrideLineNumber();
 }
 
-void StackFrame::expressionInfo(int& divot, int& startOffset, int& endOffset, unsigned& line, unsigned& column)
-{
-    codeBlock->expressionRangeForBytecodeOffset(bytecodeOffset, divot, startOffset, endOffset, line, column);
-    divot += characterOffset;
-}
-
 String StackFrame::toString(VM& vm)
 {
     StringBuilder traceBuild;
-    String functionName = friendlyFunctionName(vm);
-    String sourceURL = friendlySourceURL();
+    String functionName = this->functionName(vm);
+    String sourceURL = this->sourceURL();
     traceBuild.append(functionName);
     if (!sourceURL.isEmpty()) {
         if (!functionName.isEmpty())
             traceBuild.append('@');
         traceBuild.append(sourceURL);
-        if (codeType != StackFrameNativeCode) {
+        if (codeBlock) {
             unsigned line;
             unsigned column;
             computeLineAndColumn(line, column);
@@ -546,23 +512,18 @@
             if (visitor->isJSFrame()
                 && !isWebAssemblyExecutable(visitor->codeBlock()->ownerExecutable())
                 && !visitor->codeBlock()->unlinkedCodeBlock()->isBuiltinFunction()) {
-                CodeBlock* codeBlock = visitor->codeBlock();
                 StackFrame s = {
                     Strong<JSObject>(vm, visitor->callee()),
-                    getStackFrameCodeType(visitor),
-                    Strong<ScriptExecutable>(vm, codeBlock->ownerScriptExecutable()),
-                    Strong<UnlinkedCodeBlock>(vm, codeBlock->unlinkedCodeBlock()),
-                    codeBlock->source(),
-                    codeBlock->ownerScriptExecutable()->firstLine(),
-                    codeBlock->firstLineColumnOffset(),
-                    codeBlock->sourceOffset(),
-                    visitor->bytecodeOffset(),
-                    visitor->sourceURL(),
-                    visitor->sourceID(),
+                    Strong<CodeBlock>(vm, visitor->codeBlock()),
+                    visitor->bytecodeOffset()
                 };
                 m_results.append(s);
             } else {
-                StackFrame s = { Strong<JSObject>(vm, visitor->callee()), StackFrameNativeCode, Strong<ScriptExecutable>(), Strong<UnlinkedCodeBlock>(), 0, 0, 0, 0, 0, String(), noSourceID};
+                StackFrame s = {
+                    Strong<JSObject>(vm, visitor->callee()),
+                    Strong<CodeBlock>(),
+                    0 // unused value because codeBlock is null.
+                };
                 m_results.append(s);
             }
     

Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.h (201829 => 201830)


--- trunk/Source/_javascript_Core/interpreter/Interpreter.h	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.h	2016-06-08 20:59:49 UTC (rev 201830)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, 2013, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2008, 2013, 2015-2016 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Research In Motion Limited. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -85,23 +85,16 @@
 
     struct StackFrame {
         Strong<JSObject> callee;
-        StackFrameCodeType codeType;
-        Strong<ScriptExecutable> executable;
-        Strong<UnlinkedCodeBlock> codeBlock;
-        RefPtr<SourceProvider> code;
-        int lineOffset;
-        unsigned firstLineColumnOffset;
-        unsigned characterOffset;
+        Strong<CodeBlock> codeBlock;
         unsigned bytecodeOffset;
-        String sourceURL;
-        intptr_t sourceID;
-        String toString(VM&);
-        String friendlySourceURL() const;
-        String friendlyFunctionName(VM&) const;
-        JS_EXPORT_PRIVATE void computeLineAndColumn(unsigned& line, unsigned& column);
 
-    private:
-        void expressionInfo(int& divot, int& startOffset, int& endOffset, unsigned& line, unsigned& column);
+        bool isNative() const { return !codeBlock; }
+
+        void computeLineAndColumn(unsigned& line, unsigned& column);
+        String functionName(VM&) const;
+        intptr_t sourceID() const;
+        String sourceURL() const;
+        String toString(VM&);
     };
 
     class SuspendExceptionScope {

Modified: trunk/Source/_javascript_Core/runtime/Error.cpp (201829 => 201830)


--- trunk/Source/_javascript_Core/runtime/Error.cpp	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/runtime/Error.cpp	2016-06-08 20:59:49 UTC (rev 201830)
@@ -1,7 +1,7 @@
 /*
  *  Copyright (C) 1999-2001 Harri Porten (por...@kde.org)
  *  Copyright (C) 2001 Peter Kelly (p...@post.com)
- *  Copyright (C) 2003, 2004, 2005, 2006, 2008, 2016 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003-2006, 2008, 2016 Apple Inc. All rights reserved.
  *  Copyright (C) 2007 Eric Seidel (e...@webkit.org)
  *
  *  This library is free software; you can redistribute it and/or
@@ -139,21 +139,19 @@
     mutable unsigned m_index;
 };
 
-bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame, CallFrame*& callFrame, unsigned &bytecodeOffset) 
+bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame, CallFrame*& callFrame, unsigned* bytecodeOffset)
 {
     Vector<StackFrame> stackTrace = Vector<StackFrame>();
 
-    if (exec && stackTrace.isEmpty())
-        vm.interpreter->getStackTrace(stackTrace);
-
+    vm.interpreter->getStackTrace(stackTrace);
     if (!stackTrace.isEmpty()) {
 
         ASSERT(exec == vm.topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec());
 
-        StackFrame* stackFrame = nullptr;
+        StackFrame* firstNonNativeFrame;
         for (unsigned i = 0 ; i < stackTrace.size(); ++i) {
-            stackFrame = &stackTrace.at(i);
-            if (stackFrame->bytecodeOffset)
+            firstNonNativeFrame = &stackTrace.at(i);
+            if (!firstNonNativeFrame->isNative())
                 break;
         }
 
@@ -162,18 +160,19 @@
             vm.topCallFrame->iterate(functor);
             callFrame = functor.foundCallFrame();
             unsigned stackIndex = functor.index();
-            bytecodeOffset = stackTrace.at(stackIndex).bytecodeOffset;
+            *bytecodeOffset = stackTrace.at(stackIndex).bytecodeOffset;
         }
         
         unsigned line;
         unsigned column;
-        stackFrame->computeLineAndColumn(line, column);
+        firstNonNativeFrame->computeLineAndColumn(line, column);
         obj->putDirect(vm, vm.propertyNames->line, jsNumber(line), ReadOnly | DontDelete);
         obj->putDirect(vm, vm.propertyNames->column, jsNumber(column), ReadOnly | DontDelete);
 
-        if (!stackFrame->sourceURL.isEmpty())
-            obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, stackFrame->sourceURL), ReadOnly | DontDelete);
-    
+        String frameSourceURL = firstNonNativeFrame->sourceURL();
+        if (!frameSourceURL.isEmpty())
+            obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, frameSourceURL), ReadOnly | DontDelete);
+
         if (!useCurrentFrame)
             stackTrace.remove(0);
         obj->putDirect(vm, vm.propertyNames->stack, vm.interpreter->stackTraceAsString(vm.topCallFrame, stackTrace), DontEnum);
@@ -186,8 +185,7 @@
 void addErrorInfo(ExecState* exec, JSObject* obj, bool useCurrentFrame)
 {
     CallFrame* callFrame = nullptr;
-    unsigned bytecodeOffset = 0;
-    addErrorInfoAndGetBytecodeOffset(exec, exec->vm(), obj, useCurrentFrame, callFrame, bytecodeOffset);
+    addErrorInfoAndGetBytecodeOffset(exec, exec->vm(), obj, useCurrentFrame, callFrame);
 }
 
 JSObject* addErrorInfo(CallFrame* callFrame, JSObject* error, int line, const SourceCode& source)

Modified: trunk/Source/_javascript_Core/runtime/Error.h (201829 => 201830)


--- trunk/Source/_javascript_Core/runtime/Error.h	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/runtime/Error.h	2016-06-08 20:59:49 UTC (rev 201830)
@@ -63,7 +63,7 @@
 JS_EXPORT_PRIVATE JSObject* createOutOfMemoryError(ExecState*);
 
 
-bool addErrorInfoAndGetBytecodeOffset(ExecState*, VM&, JSObject*, bool, CallFrame*&, unsigned&);
+bool addErrorInfoAndGetBytecodeOffset(ExecState*, VM&, JSObject*, bool, CallFrame*&, unsigned* = nullptr);
 
 bool hasErrorInfo(ExecState*, JSObject* error);
 JS_EXPORT_PRIVATE void addErrorInfo(ExecState*, JSObject*, bool); 

Modified: trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp (201829 => 201830)


--- trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp	2016-06-08 20:56:11 UTC (rev 201829)
+++ trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp	2016-06-08 20:59:49 UTC (rev 201830)
@@ -142,9 +142,9 @@
     if (!message.isNull())
         putDirect(vm, vm.propertyNames->message, jsString(&vm, message), DontEnum);
 
-    unsigned bytecodeOffset = hasSourceAppender();
+    unsigned bytecodeOffset = 0;
     CallFrame* callFrame = nullptr;
-    bool hasTrace = addErrorInfoAndGetBytecodeOffset(exec, vm, this, useCurrentFrame, callFrame, bytecodeOffset);
+    bool hasTrace = addErrorInfoAndGetBytecodeOffset(exec, vm, this, useCurrentFrame, callFrame, hasSourceAppender() ? &bytecodeOffset : nullptr);
 
     if (hasTrace && callFrame && hasSourceAppender()) {
         if (callFrame && callFrame->codeBlock()) 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to