Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 2563673002: [wasm] Generate correct locations for error messages (Closed)

Created:
4 years ago by Clemens Hammacher
Modified:
4 years ago
Reviewers:
titzer, Yang
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] Generate correct locations for error messages The current logic in Isolate::GetLocationFromStackTrace just ignores wasm frames, making the computed location point to the first javascript frame, like this: test.js:17: RuntimeError: divide by zero module.exports.main(); ^ RuntimeError: divide by zero at main (<WASM>[1]+5) at test.js:17:16 This CL not only fixes the location to point to the top-most wasm frame, but also exposes to the embedder that the script of that location is a wasm script, allowing for custom printing of wasm locations. The Shell::ReportException method now checks for this flag, and prints wasm locations like this: <WASM>[0]+5: RuntimeError: divide by zero RuntimeError: divide by zero at main (<WASM>[0]+5) at test/message/wasm-trap.js:15:16 R=titzer@chromium.org, yangguo@chromium.org BUG=chromium:613110 Committed: https://crrev.com/222541dff51aba0623edc98bbecb6e1a35d22063 Cr-Commit-Position: refs/heads/master@{#41640}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -44 lines) Patch
M include/v8.h View 1 3 chunks +11 lines, -7 lines 0 comments Download
M src/api.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/d8.h View 1 chunk +0 lines, -1 line 0 comments Download
M src/d8.cc View 3 chunks +22 lines, -23 lines 0 comments Download
M src/isolate.cc View 1 1 chunk +18 lines, -12 lines 0 comments Download
M src/messages.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
A test/message/wasm-trap.js View 1 chunk +15 lines, -0 lines 0 comments Download
A test/message/wasm-trap.out View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (13 generated)
Clemens Hammacher
4 years ago (2016-12-08 15:26:02 UTC) #3
Clemens Hammacher
Ping?
4 years ago (2016-12-12 09:35:47 UTC) #6
titzer
lgtm
4 years ago (2016-12-12 11:51:04 UTC) #7
titzer
https://codereview.chromium.org/2563673002/diff/1/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/2563673002/diff/1/src/d8.cc#newcode1260 src/d8.cc:1260: auto ToCString = [](const v8::String::Utf8Value& value) { Finally a ...
4 years ago (2016-12-12 11:51:33 UTC) #8
Yang
On 2016/12/12 11:51:04, titzer wrote: > lgtm lgtm.
4 years ago (2016-12-12 11:52:08 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2563673002/20001
4 years ago (2016-12-12 12:17:56 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-12 12:45:55 UTC) #19
commit-bot: I haz the power
4 years ago (2016-12-12 12:46:10 UTC) #21
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/222541dff51aba0623edc98bbecb6e1a35d22063
Cr-Commit-Position: refs/heads/master@{#41640}

Powered by Google App Engine
This is Rietveld 408576698