Bug 165429

Summary:

We should be able to throw exceptions from Wasm code and when Wasm frames are on the stack

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, cgarcia, clopez, commit-queue, fpizlo, ggaren, gskachkov, gyuyoung.kim, jfbastien, keith_miller, mark.lam, msaboff, oliver, ossy, rniwa, ticaiolima, ysuzuki

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

163351

Attachments:

Description	Flags
WIP	none
WIP	none
WIP	none
patch	none
patch	none
patch	keith_miller: review+
patch for landing	none
patch for landing	none
patch for landing	none
patch for landing	none
patch for landing	none
patch for landing	none
patch for landing	buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-yosemite-wk2	none
Archive of layout-test-results from ews113 for mac-yosemite	none
patch for landing	buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite	none
patch for landing	none

Saam Barati

Reported 2016-12-05 15:54:30 PST

...

Attachments
WIP (73.48 KB, patch) 2016-12-05 20:18 PST, Saam Barati	no flags	Details Formatted Diff Diff
WIP (83.17 KB, patch) 2016-12-06 14:33 PST, Saam Barati	no flags	Details Formatted Diff Diff
WIP (65.80 KB, patch) 2016-12-09 18:19 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch (73.26 KB, patch) 2016-12-09 18:53 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch (71.88 KB, patch) 2016-12-09 19:00 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch (72.63 KB, patch) 2016-12-09 19:09 PST, Saam Barati	keith_miller: review+	Details Formatted Diff Diff
patch for landing (76.82 KB, patch) 2016-12-10 01:00 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.65 KB, patch) 2016-12-10 01:06 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.90 KB, patch) 2016-12-10 01:10 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.09 KB, patch) 2016-12-10 01:17 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.19 KB, patch) 2016-12-10 01:20 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.22 KB, patch) 2016-12-10 01:27 PST, Saam Barati	no flags	Details Formatted Diff Diff
patch for landing (76.24 KB, patch) 2016-12-10 01:52 PST, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.18 MB, application/zip) 2016-12-10 03:17 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews113 for mac-yosemite (2.13 MB, application/zip) 2016-12-10 03:26 PST, Build Bot	no flags	Details
patch for landing (76.42 KB, patch) 2016-12-10 13:47 PST, Saam Barati	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.34 MB, application/zip) 2016-12-10 17:24 PST, Build Bot	no flags	Details
patch for landing (76.36 KB, patch) 2016-12-11 14:44 PST, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2016-12-05 20:18:59 PST

Created attachment 296254 [details] WIP Most things are in place now, I just need to teach StackVisitor about wasm.

Saam Barati

Comment 2 2016-12-06 14:33:24 PST

Created attachment 296323 [details] WIP Just threw a wasm exception: TypeError: Out of bounds memory access ------------------------ <wasm>@[wasm code] <wasm>@[wasm code] testWasmModuleFunctions@[native code] module code@/Volumes/Data/WK/a/OpenSource/JSTests/wasm/function-tests/i32-load.js:27:32 evaluate@[native code] moduleEvaluation@[native code] [native code] promiseReactionJob@[native code] ------------------------

Saam Barati

Comment 3 2016-12-09 18:19:39 PST

Created attachment 296760 [details] WIP might be done. I need to review the code and write a changelog.

Saam Barati

Comment 4 2016-12-09 18:53:39 PST

Created attachment 296765 [details] patch

Saam Barati

Comment 5 2016-12-09 19:00:13 PST

Created attachment 296767 [details] patch rebased

WebKit Commit Bot

Comment 6 2016-12-09 19:01:59 PST

Attachment 296767 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 5 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 7 2016-12-09 19:09:05 PST

Created attachment 296768 [details] patch fix merge conflict compile error.

WebKit Commit Bot

Comment 8 2016-12-09 19:11:00 PST

Attachment 296768 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 5 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 9 2016-12-09 19:15:48 PST

View in context: https://bugs.webkit.org/attachment.cgi?id=296767&action=review lgtm, but I'm not super familiar with this code so it would be good to have another look. > Source/JavaScriptCore/API/JSContextRef.cpp:276 > + if (visitor->hasLineAndColumnInfo()) { Should we have a fix to collect wasm function number and index? > Source/JavaScriptCore/interpreter/StackVisitor.cpp:162 > +#if ENABLE(WEBASSEMBLY) Do you need these #if here and elsewhere? You're not using anything which isn't available if not wasm. > Source/JavaScriptCore/interpreter/StackVisitor.cpp:329 > + // FIXME: This seems fine, why FIXME? > Source/JavaScriptCore/runtime/JSCell.cpp:301 > + return inherits(JSWebAssemblyCallee::info()) || inherits(WebAssemblyToJSCallee::info()); I added a function which does this in Repatch.cpp, we should share it.

Saam Barati

Comment 10 2016-12-09 20:23:06 PST

Looks like WebCore uses StackVisitor.

Keith Miller

Comment 11 2016-12-09 22:52:23 PST

Comment on attachment 296768 [details] patch I think this seems reasonable assuming you fix the WebCore build issues. Although, I don't fully understand why we would export StackVisitor to WebCore. It also, looks like you need to fix the Wasm-less builds too.

Saam Barati

Comment 12 2016-12-10 01:00:15 PST

Created attachment 296788 [details] patch for landing

WebKit Commit Bot

Comment 13 2016-12-10 01:02:26 PST

Attachment 296788 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 5 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 14 2016-12-10 01:06:10 PST

Created attachment 296791 [details] patch for landing

Saam Barati

Comment 15 2016-12-10 01:10:59 PST

Created attachment 296792 [details] patch for landing

WebKit Commit Bot

Comment 16 2016-12-10 01:12:05 PST

Attachment 296792 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:47: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/StackVisitor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSCell.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 6 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 17 2016-12-10 01:17:25 PST

Created attachment 296794 [details] patch for landing

Saam Barati

Comment 18 2016-12-10 01:20:10 PST

Created attachment 296795 [details] patch for landing

WebKit Commit Bot

Comment 19 2016-12-10 01:22:56 PST

Attachment 296795 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 20 2016-12-10 01:27:36 PST

Created attachment 296796 [details] patch for landing

WebKit Commit Bot

Comment 21 2016-12-10 01:29:57 PST

Attachment 296796 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 22 2016-12-10 01:52:11 PST

Created attachment 296798 [details] patch for landing

WebKit Commit Bot

Comment 23 2016-12-10 01:53:09 PST

Attachment 296798 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 24 2016-12-10 03:17:34 PST

Comment on attachment 296798 [details] patch for landing Attachment 296798 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2687180 New failing tests: inspector/codemirror/prettyprinting-css-rules.html

Build Bot

Comment 25 2016-12-10 03:17:38 PST

Created attachment 296799 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 26 2016-12-10 03:26:01 PST

Comment on attachment 296798 [details] patch for landing Attachment 296798 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2687207 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html js/dom/Promise-static-all.html imported/w3c/web-platform-tests/streams/byte-length-queuing-strategy.https.html streams/readable-stream-pipeThrough.html imported/w3c/web-platform-tests/streams/count-queuing-strategy.https.html imported/w3c/web-platform-tests/streams/readable-streams/pipe-through.https.html js/dom/Promise-static-race.html

Build Bot

Comment 27 2016-12-10 03:26:06 PST

Created attachment 296800 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5

Saam Barati

Comment 28 2016-12-10 13:47:42 PST

Created attachment 296822 [details] patch for landing

WebKit Commit Bot

Comment 29 2016-12-10 13:50:07 PST

Attachment 296822 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 30 2016-12-10 17:24:02 PST

Comment on attachment 296822 [details] patch for landing Attachment 296822 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2692102 New failing tests: inspector/codemirror/prettyprinting-javascript.html

Build Bot

Comment 31 2016-12-10 17:24:07 PST

Created attachment 296834 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Saam Barati

Comment 32 2016-12-11 14:34:44 PST

(In reply to comment #31) > Created attachment 296834 [details] > Archive of layout-test-results from ews103 for mac-yosemite > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5 Hmmm, the crashes here don't look related to my patch. I'm going to rebase the patch and re-upload for EWS.

Saam Barati

Comment 33 2016-12-11 14:44:07 PST

Created attachment 296874 [details] patch for landing

WebKit Commit Bot

Comment 34 2016-12-11 14:45:52 PST

Attachment 296874 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:188: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Commit Bot

Comment 35 2016-12-11 19:11:51 PST

Comment on attachment 296874 [details] patch for landing Clearing flags on attachment: 296874 Committed r209696: <http://trac.webkit.org/changeset/209696>

WebKit Commit Bot

Comment 36 2016-12-11 19:11:58 PST

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 37 2016-12-11 23:17:57 PST

(In reply to comment #35) > Comment on attachment 296874 [details] > patch for landing > > Clearing flags on attachment: 296874 > > Committed r209696: <http://trac.webkit.org/changeset/209696> It broke the Linux builds, see build.webkit.org for details.

Saam Barati

Comment 38 2016-12-11 23:39:46 PST

(In reply to comment #37) > (In reply to comment #35) > > Comment on attachment 296874 [details] > > patch for landing > > > > Clearing flags on attachment: 296874 > > > > Committed r209696: <http://trac.webkit.org/changeset/209696> > > It broke the Linux builds, see build.webkit.org for details. About to land a speculative fix

Saam Barati

Comment 39 2016-12-11 23:47:12 PST

Tried to fix the build in: https://trac.webkit.org/changeset/209702

Csaba Osztrogonác

Comment 40 2016-12-12 03:56:35 PST

cloop build is still broken: https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/22207

Saam Barati

Comment 41 2016-12-12 12:56:20 PST

(In reply to comment #40) > cloop build is still broken: > https://build.webkit.org/builders/ > Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/22207 Fix in: https://trac.webkit.org/changeset/209722

Note You need to log in before you can comment on or make changes to this bug.