Skip to content

Fix runtime stack trace computation - #11581

Merged
Jarred-Sumner merged 12 commits into
mainfrom
dave/runtime-stack-trace
Jun 5, 2024
Merged

Jarred-Sumner merged 12 commits into
mainfrom
dave/runtime-stack-trace

Conversation

@paperclover

@paperclover paperclover commented Jun 4, 2024 •

Copy link
Copy Markdown
Contributor

Fixes #9302
Fixes #8880

After the previous adventure of source-mapping related issues, it seems the big issue blocking sourcemaps from working is simply our code reading line+column numbers from JavaScriptCore.

The original code

-   /* Get the "raw" position info.
-    * Note that we're using m_codeBlock->unlinkedCodeBlock()->expressionRangeForBytecodeOffset
-    * rather than m_codeBlock->expressionRangeForBytecodeOffset in order get the "raw" offsets and
-    * avoid the CodeBlock's expressionRangeForBytecodeOffset modifications to the line and column
-    * numbers, (we don't need the column number from it, and we'll calculate the line "fixes"
-    * ourselves). */
-   ExpressionInfo::Entry info = m_codeBlock->unlinkedCodeBlock()->expressionInfoForBytecodeIndex(bytecodeOffset);

Is an alright idea, but the code that handles fixing this raw info does not work, with cases as simple as:

// @bun
var l = Object.create; (() => { throw new Error() })();

Instead we will simply use expressionInfoForBytecodeIndex on the linked code block to retrive all of the computed offsets.

+auto expr = m_codeBlock->expressionInfoForBytecodeIndex(bytecodeOffset);

And to prevent zero-based vs one-based, I am using OrdinalNumber practically everywhere to ensure these numbers are interpretted correctly.

Draft PR because there are surely test failures which will need to be corrected with proper location info, but I am noticing some potential mistakes in regards to oneBasedInt and zeroBasedInt

@paperclover

This comment was marked as resolved.

@github-actions

github-actions Bot commented Jun 4, 2024 •

Copy link
Copy Markdown
Contributor

❌ @paperdave, your commit has failing tests :(

💻 1 failing tests Darwin x64 baseline

  • test/js/web/workers/worker.test.ts 1 failing

💻 1 failing tests Darwin x64

  • test/cli/install/bun-create.test.ts 1 failing

🪟💻 4 failing tests Windows x64 baseline

  • test/cli/install/bun-create.test.ts 1 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/js/node/watch/fs.watchFile.test.ts 3 failing

🪟💻 3 failing tests Windows x64

  • test/cli/install/bunx.test.ts 1 failing
  • test/integration/next-pages/test/dev-server.test.ts 1 failing
  • test/js/node/watch/fs.watchFile.test.ts 3 failing

View logs

@paperclover

Copy link
Copy Markdown
Contributor Author

This seems to move the column number from the new keyword to the Error function name.

image

@paperclover

Copy link
Copy Markdown
Contributor Author

This seems to move the column number from the new keyword to the Error function name.

This one is actually insane.
image
Look at this sourcemap, if the Error( is one source mapping token, then it means a divot pointing at the ( will remap to point at the E in Error, which is what we observe.

The old logic was either

  • bugged in just the right way to work
  • we had an off-by-one token issue

I'm going to work on getting the sourcemap columns on function calls to just work. Matching v8 is so close i can taste it.

int32_t column_stop;
int32_t expression_start;
int32_t expression_stop;
WTF::OrdinalNumber line;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to use WTF::OrdinalNumber here? It won't be initialized as an C++ WTF::OrdinalNumber in Zig. I think this should be stored as an int32_t and then a method could be added to convert it to a WTF::OrdinalNumber.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah lets do this

int32_t expression_stop;
WTF::OrdinalNumber line;
WTF::OrdinalNumber column;
int byte_position;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be more specific so there's no ambiguity:

Suggested change
int byte_position;
int32_t byte_position;

typedef struct ZigStackTrace {
BunString* source_lines_ptr;
int32_t* source_lines_numbers;
OrdinalNumber* source_lines_numbers;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't share C++ classes in Zig

Suggested change
OrdinalNumber* source_lines_numbers;
int32_t* source_lines_numbers;

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple nitpicky comments but this is a good improvement and once the tests pass & the comments are addressed, we should merge

@Jarred-Sumner
Jarred-Sumner marked this pull request as ready for review June 5, 2024 00:25

pos.column_zero_based = pos.column_zero_based - amount;
if (pos.column_zero_based < 0) {
auto source = code->source().provider()->source();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto source = code->source().provider()->source();
const auto *provider = code->source().provider();
if (UNLIKELY(!provider)) {
return;
}
const auto& source = provider->source();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Source maps and error locations are broken assert points to the wrong line

2 participants