Bug 170135

I think this is done in bug #169773?

(In reply to JF Bastien from comment #1) > I think this is done in bug #169773? That patch does not make callee not a JSCell.

Created attachment 305542 [details] WIP Doesn't compile yet

I'm also decoupling CCallHelpers/AssemblyHelpers from a VM in this patch. Any function that does VM-y things, will take a VM& as a parameter.

Created attachment 305549 [details] WIP Still doesn't compile

Created attachment 305555 [details] WIP

I think everything else is in place for this to happen.

Created attachment 306124 [details] WIP It's still crashing on some wasm tests. I'm re-writing the various runtime bits that wasm uses to make sure they don't use exec->vm() because that assumes we have a JSCell Callee

Created attachment 306143 [details] WIP passes wasm tests.

Created attachment 306152 [details] WIP Catching an exception w/ web inspector open no longer crashes. But it still doesn't show any wasm frames. We should fix that soon.

Created attachment 306157 [details] WIP Almost done, just need to rename some files.

Created attachment 306164 [details] patch

Attachment 306164 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:59: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:190: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/interpreter/CalleeBits.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/wasm/WasmPlan.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/JavaScriptCore/wasm/WasmContext.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 7 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 306164 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306164&action=review Some tests are sad. Looks good overall, thought I'm not familiar with some of the code so it would be good to have another set of eyes looking. > Source/JavaScriptCore/jsc.cpp:3206 > + Vector<Ref<Wasm::Callee>> keepAlive; It's not obvious what the difference is between these two from just their name. I know it's pre-existing, but still confusing IMO. > Source/JavaScriptCore/wasm/WasmCallee.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2017 > Source/JavaScriptCore/wasm/WasmCallee.h:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2017 > Source/JavaScriptCore/wasm/WasmPlanInlines.h:43 > + return bitwise_cast<void*>(bitwise_cast<uintptr_t>(callee) | 1); Could you keep the | 1 with the same code that does & ~1 ? That would prevent it from ever diverging, or being missed in a copy-paste tragedy. > Source/JavaScriptCore/wasm/WasmThunks.cpp:76 > + // this since it is compiled with a constant VM pointer.) We could make the calling convetion Typo "convention" > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:79 > + // These two callee getters are only valid once the fields have been polulated. Typo "populated" > Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:35 > +const ClassInfo JSWebAssemblyRuntimeError::s_info = { "WebAssembly.RuntimeError", &Base::s_info, 0, CREATE_METHOD_TABLE(JSWebAssemblyRuntimeError) }; The other two wasm error types were auto-generated together and should have the same position as here. I'd rather update all or none.

Looks like there are some 32 bit build errors.

Comment on attachment 306164 [details] patch Will work on 32-bit

Comment on attachment 306164 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306164&action=review >> Source/JavaScriptCore/jsc.cpp:3206 >> + Vector<Ref<Wasm::Callee>> keepAlive; > > It's not obvious what the difference is between these two from just their name. I know it's pre-existing, but still confusing IMO. Ok. I'll name them better. >> Source/JavaScriptCore/wasm/WasmCallee.h:2 >> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved. > > 2017 Why? This file was "svn mv"ed. >> Source/JavaScriptCore/wasm/WasmPlanInlines.h:43 >> + return bitwise_cast<void*>(bitwise_cast<uintptr_t>(callee) | 1); > > Could you keep the | 1 with the same code that does & ~1 ? That would prevent it from ever diverging, or being missed in a copy-paste tragedy. Yeah definitely. Will fix this.

Created attachment 306184 [details] patch

Attachment 306184 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:190: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/interpreter/CalleeBits.h:42: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 306184 [details] patch r=me

landed in: https://trac.webkit.org/changeset/214905/webkit