From 4b71c9d2b57fab59c652861c2077f0eed5ef5b62 Mon Sep 17 00:00:00 2001 From: "mark.lam@apple.com" Date: Wed, 8 Jun 2016 20:59:49 +0000 Subject: [PATCH] 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): git-svn-id: https://svn.webkit.org/repository/webkit/trunk@201830 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/JavaScriptCore/ChangeLog | 61 ++++++++++ .../inspector/ScriptCallStackFactory.cpp | 8 +- Source/JavaScriptCore/interpreter/Interpreter.cpp | 127 +++++++-------------- Source/JavaScriptCore/interpreter/Interpreter.h | 25 ++-- Source/JavaScriptCore/runtime/Error.cpp | 28 +++-- Source/JavaScriptCore/runtime/Error.h | 2 +- Source/JavaScriptCore/runtime/ErrorInstance.cpp | 4 +- 7 files changed, 134 insertions(+), 121 deletions(-) diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index ce3d211..33bfd1e 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,64 @@ +2016-06-08 Mark Lam + + 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 We should be able to lookup symbols by identifier in builtins diff --git a/Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp b/Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp index d9d4af9..b639d11 100644 --- a/Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp +++ b/Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp @@ -143,8 +143,8 @@ Ref createScriptCallStackFromException(JSC::ExecState* exec, JS unsigned line; unsigned column; stackTrace[i].computeLineAndColumn(line, column); - String functionName = stackTrace[i].friendlyFunctionName(vm); - frames.append(ScriptCallFrame(functionName, stackTrace[i].friendlySourceURL(), static_cast(stackTrace[i].sourceID), line, column)); + String functionName = stackTrace[i].functionName(vm); + frames.append(ScriptCallFrame(functionName, stackTrace[i].sourceURL(), static_cast(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 @@ Ref createScriptCallStackFromException(JSC::ExecState* exec, JS 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); } } } diff --git a/Source/JavaScriptCore/interpreter/Interpreter.cpp b/Source/JavaScriptCore/interpreter/Interpreter.cpp index 04740cc..01dc15d 100644 --- a/Source/JavaScriptCore/interpreter/Interpreter.cpp +++ b/Source/JavaScriptCore/interpreter/Interpreter.cpp @@ -87,49 +87,44 @@ using namespace std; 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 @@ bool Interpreter::isOpcode(Opcode opcode) #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 @@ void StackFrame::computeLineAndColumn(unsigned& line, unsigned& column) int divot = 0; int unusedStartOffset = 0; int unusedEndOffset = 0; - unsigned divotLine = 0; - unsigned divotColumn = 0; - expressionInfo(divot, unusedStartOffset, unusedEndOffset, divotLine, divotColumn); - - line = divotLine + lineOffset; - column = divotColumn + (divotLine ? 1 : firstLineColumnOffset); + codeBlock->expressionRangeForBytecodeOffset(bytecodeOffset, divot, unusedStartOffset, unusedEndOffset, line, column); + 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 @@ public: if (visitor->isJSFrame() && !isWebAssemblyExecutable(visitor->codeBlock()->ownerExecutable()) && !visitor->codeBlock()->unlinkedCodeBlock()->isBuiltinFunction()) { - CodeBlock* codeBlock = visitor->codeBlock(); StackFrame s = { Strong(vm, visitor->callee()), - getStackFrameCodeType(visitor), - Strong(vm, codeBlock->ownerScriptExecutable()), - Strong(vm, codeBlock->unlinkedCodeBlock()), - codeBlock->source(), - codeBlock->ownerScriptExecutable()->firstLine(), - codeBlock->firstLineColumnOffset(), - codeBlock->sourceOffset(), - visitor->bytecodeOffset(), - visitor->sourceURL(), - visitor->sourceID(), + Strong(vm, visitor->codeBlock()), + visitor->bytecodeOffset() }; m_results.append(s); } else { - StackFrame s = { Strong(vm, visitor->callee()), StackFrameNativeCode, Strong(), Strong(), 0, 0, 0, 0, 0, String(), noSourceID}; + StackFrame s = { + Strong(vm, visitor->callee()), + Strong(), + 0 // unused value because codeBlock is null. + }; m_results.append(s); } diff --git a/Source/JavaScriptCore/interpreter/Interpreter.h b/Source/JavaScriptCore/interpreter/Interpreter.h index d79069a..3b8e7ee 100644 --- a/Source/JavaScriptCore/interpreter/Interpreter.h +++ b/Source/JavaScriptCore/interpreter/Interpreter.h @@ -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 @@ namespace JSC { struct StackFrame { Strong callee; - StackFrameCodeType codeType; - Strong executable; - Strong codeBlock; - RefPtr code; - int lineOffset; - unsigned firstLineColumnOffset; - unsigned characterOffset; + Strong 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 { diff --git a/Source/JavaScriptCore/runtime/Error.cpp b/Source/JavaScriptCore/runtime/Error.cpp index 88f8848..f267124 100644 --- a/Source/JavaScriptCore/runtime/Error.cpp +++ b/Source/JavaScriptCore/runtime/Error.cpp @@ -1,7 +1,7 @@ /* * Copyright (C) 1999-2001 Harri Porten (porten@kde.org) * Copyright (C) 2001 Peter Kelly (pmk@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 (eric@webkit.org) * * This library is free software; you can redistribute it and/or @@ -139,21 +139,19 @@ private: 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 stackTrace = Vector(); - 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 @@ bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bo 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 @@ bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bo 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) diff --git a/Source/JavaScriptCore/runtime/Error.h b/Source/JavaScriptCore/runtime/Error.h index c511d69..1e5857d 100644 --- a/Source/JavaScriptCore/runtime/Error.h +++ b/Source/JavaScriptCore/runtime/Error.h @@ -63,7 +63,7 @@ JS_EXPORT_PRIVATE JSObject* createURIError(ExecState*, const String&); 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); diff --git a/Source/JavaScriptCore/runtime/ErrorInstance.cpp b/Source/JavaScriptCore/runtime/ErrorInstance.cpp index 1642582..95fd363 100644 --- a/Source/JavaScriptCore/runtime/ErrorInstance.cpp +++ b/Source/JavaScriptCore/runtime/ErrorInstance.cpp @@ -142,9 +142,9 @@ void ErrorInstance::finishCreation(ExecState* exec, VM& vm, const String& messag 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()) -- 1.8.3.1