Repository navigation
node:vm: apply negative lineOffset/columnOffset to stack frames and the error header #38248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d8fb407
35feb71
1c41504
d4ab733
943e04b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,31 @@ | ||
| #include "root.h" | ||
| #include "ErrorStackFrame.h" | ||
| #include "JavaScriptCore/CodeBlock.h" | ||
| #include "headers-handwritten.h" | ||
| #include "JavaScriptCore/BytecodeIndex.h" | ||
| #include "JavaScriptCore/StackFrame.h" | ||
| #include "wtf/Assertions.h" | ||
| #include "wtf/text/OrdinalNumber.h" | ||
|
|
||
| namespace Bun { | ||
| using namespace JSC; | ||
|
|
||
| void applyNegativeSourceStart(CodeBlock* code, int& lineZeroBased, int& columnZeroBased) | ||
| { | ||
| auto* provider = code->source().provider(); | ||
| if (!provider) | ||
| return; | ||
|
|
||
| // Nested functions share the program's provider, so this is the whole source's start. | ||
| TextPosition start = provider->startPosition(); | ||
| int startLine = start.m_line.zeroBasedInt(); | ||
| int startColumn = start.m_column.zeroBasedInt(); | ||
|
|
||
| // JSC applies the start column to the first physical line only, reported as the clamped start line. | ||
| if (startColumn < 0 && lineZeroBased == std::max(startLine, 0)) | ||
| columnZeroBased += startColumn; | ||
| if (startLine < 0) | ||
| lineZeroBased += startLine; | ||
| } | ||
|
|
||
| /// Adjust a `ZigStackFramePosition` by a number of bytes. This accounts for when the adjustment | ||
| /// crosses line boundaries, and thus requires the source code in order to properly compute | ||
| /// the result. | ||
|
|
@@ -95,6 +113,20 @@ | |
| break; | ||
| } | ||
|
|
||
| // After the walk: it works in JSC's coordinates and may end on a different line. | ||
| applyNegativeSourceStart(code, pos.line_zero_based, pos.column_zero_based); | ||
| return pos; | ||
|
Check warning on line 118 in src/jsc/bindings/ErrorStackFrame.cpp
|
||
|
Comment on lines
+116
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Extended reasoning...What the bug is
Bun's error printer in let col = top.position.column.zero_based();
...
let indent = max_line_number_pad + b" | ".len() as u64 + col.max(0) as u64;
splat_space(writer, indent)?;
pretty_write!(writer, "<red><b>^<r>\n")?;
Concrete examplenew (require("vm").Script)(" throw new Error('x')", { filename: "c.js", columnOffset: -3 })
.runInThisContext({ displayErrors: false });Left uncaught (or The Why this is new for negative offsets, but the class is pre-existingBefore this PR, JSC's However, the same misalignment already existed for positive Why the tests don't catch itThe PR's "caret stays on the text" test with FixEither have the source-excerpt renderer subtract the same |
||
| } | ||
|
|
||
| ZigStackFramePosition getLineColumnForStackFrame(const StackFrame& frame) | ||
| { | ||
| auto lineColumn = frame.computeLineAndColumn(); | ||
| ZigStackFramePosition pos { | ||
| .line_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.line).zeroBasedInt(), | ||
| .column_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.column).zeroBasedInt(), | ||
| .byte_position = -1, | ||
| }; | ||
| applyNegativeSourceStart(frame.codeBlock(), pos.line_zero_based, pos.column_zero_based); | ||
| return pos; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include "root.h" | ||
| #include "headers-handwritten.h" | ||
| #include "JavaScriptCore/BytecodeIndex.h" | ||
|
|
||
| namespace JSC { | ||
| class CodeBlock; | ||
| class StackFrame; | ||
| } | ||
|
|
||
| namespace Bun { | ||
|
|
||
| ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc); | ||
|
|
||
| /// Re-adds the part of a negative node:vm lineOffset / columnOffset that JSC::SourceCode clamps away; no-op for other sources. | ||
| void applyNegativeSourceStart(JSC::CodeBlock* code, int& lineZeroBased, int& columnZeroBased); | ||
|
|
||
| /// StackFrame::computeLineAndColumn() of a frame with a code block, with the above applied. | ||
| ZigStackFramePosition getLineColumnForStackFrame(const JSC::StackFrame& frame); | ||
|
|
||
| } // namespace Bun |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
|
|
||
| #include "BunClientData.h" | ||
| #include "CallSite.h" | ||
| #include "ErrorStackFrame.h" | ||
| #include "ErrorStackTrace.h" | ||
| #include "headers-handwritten.h" | ||
|
|
||
|
|
@@ -243,11 +244,11 @@ | |
| // file's map once instead of per frame. | ||
| WTF::Vector<ZigStackFrame, 8> remappedFrames; | ||
| WTF::Vector<WTF::String, 8> sourceURLs; | ||
| WTF::Vector<LineColumn, 8> originalLineColumns; | ||
| WTF::Vector<ZigStackFramePosition, 8> originalPositions; | ||
| remappedFrames.grow(framesCount); | ||
| memset(remappedFrames.begin(), 0, sizeof(ZigStackFrame) * framesCount); | ||
| sourceURLs.grow(framesCount); | ||
| originalLineColumns.grow(framesCount); | ||
| originalPositions.grow(framesCount); | ||
| bool anyRemap = false; | ||
|
|
||
| for (size_t i = 0; i < framesCount; i++) { | ||
|
|
@@ -260,11 +261,11 @@ | |
| remappedFrame.position.line_zero_based = -1; | ||
| remappedFrame.position.column_zero_based = -1; | ||
| remappedFrame.position.byte_position = -1; | ||
| originalLineColumns[i] = {}; | ||
| originalPositions[i] = remappedFrame.position; | ||
|
|
||
| if (!frame.hasLineAndColumnInfo()) continue; | ||
|
|
||
| originalLineColumns[i] = frame.computeLineAndColumn(); | ||
| originalPositions[i] = Bun::getLineColumnForStackFrame(frame); | ||
|
Check warning on line 268 in src/jsc/bindings/FormatStackTraceForJS.cpp
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 For a line-1 frame whose negative Extended reasoning...What the bug is
if (displayLine.zeroBasedInt() > 0 || displayColumn.zeroBasedInt() > 0) {
sb.append(':');
sb.append(displayLine.oneBasedInt());
if (displayColumn.zeroBasedInt() > 0) { ... }
}That gate is pre-existing, but this PR changes what feeds it. Line 268 now sets The specific code pathFor a frame on the first physical line (JSC reports it as one-based line 1, i.e. Step-by-step exampletry {
new (require('vm').Script)("throw new Error('x')", { filename: 'c.js', columnOffset: -100 }).runInThisContext();
} catch (e) { console.log(e.stack); }
Node prints Why the PR's tests miss itEvery negative- Impact and why it's a nitThe PR description says "error.stack and CallSite#toJSON carry them as V8 would ( FixEither change the outer gate to test |
||
|
|
||
| JSC::JSGlobalObject* globalObjectForFrame = lexicalGlobalObject; | ||
| if (auto* callee = frame.callee()) { | ||
|
|
@@ -280,8 +281,7 @@ | |
| if (isDefinitelyNotRunninginNodeVMGlobalObject || isDefaultGlobalObjectInAFinalizer) { | ||
| // https://github.com/oven-sh/bun/issues/3595 | ||
| if (!sourceURLs[i].isEmpty()) { | ||
| remappedFrame.position.line_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line).zeroBasedInt(); | ||
| remappedFrame.position.column_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column).zeroBasedInt(); | ||
| remappedFrame.position = originalPositions[i]; | ||
| remappedFrame.source_url = Bun::toStringRef(sourceURLs[i]); | ||
| anyRemap = true; | ||
| } | ||
|
|
@@ -316,8 +316,8 @@ | |
| WTF::String sourceURLForFrame = sourceURLs[i]; | ||
|
|
||
| if (frame.hasLineAndColumnInfo()) { | ||
| originalLine = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line); | ||
| originalColumn = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column); | ||
| originalLine = originalPositions[i].line(); | ||
| originalColumn = originalPositions[i].column(); | ||
| displayLine = originalLine; | ||
| displayColumn = originalColumn; | ||
|
|
||
|
|
@@ -575,6 +575,12 @@ | |
| return computeErrorInfoToJSValueWithoutSkipping(vm, stackTrace, line, column, sourceURL, errorInstance, bunErrorData); | ||
| } | ||
|
|
||
| // ErrorInstance keeps error.line / error.column unsigned; a node:vm offset can put a position at or below 0, which reads as unset (0) there. | ||
| static unsigned toUnsignedOneBased(OrdinalNumber ordinal) | ||
| { | ||
| return static_cast<unsigned>(std::max(ordinal.oneBasedInt(), 0)); | ||
| } | ||
|
|
||
| WTF::String computeErrorInfoWrapperToString(JSC::VM& vm, Vector<StackFrame>& stackTrace, unsigned int& line_in, unsigned int& column_in, String& sourceURL, void* bunErrorData) | ||
| { | ||
| UNUSED_PARAM(bunErrorData); | ||
|
|
@@ -607,8 +613,8 @@ | |
| result = WTF::emptyString(); | ||
| } | ||
|
|
||
| line_in = line.oneBasedInt(); | ||
| column_in = column.oneBasedInt(); | ||
| line_in = toUnsignedOneBased(line); | ||
| column_in = toUnsignedOneBased(column); | ||
|
|
||
| return result; | ||
| } | ||
|
|
@@ -644,8 +650,8 @@ | |
|
|
||
| JSValue result = computeErrorInfoToJSValue(vm, stackTrace, line, column, sourceURL, errorInstance, bunErrorData); | ||
|
|
||
| line_in = line.oneBasedInt(); | ||
| column_in = column.oneBasedInt(); | ||
| line_in = toUnsignedOneBased(line); | ||
| column_in = toUnsignedOneBased(column); | ||
|
|
||
| // materializeErrorInfoIfNeeded putDirect()s this unconditionally; an empty JSValue | ||
| // in property storage crashes the next read. https://github.com/oven-sh/bun/issues/34095 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.