Bug 183442

Created attachment 337019 [details] Patch WiP. Patch without tests. Current patch crash during WebAssembly.instantiateStreaming in case of large wasm modules

Attachment 337019 [details] did not pass style-queue: 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] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 337296 [details] Patch Patch for review

Comment on attachment 337296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337296&action=review r- given that EWS bots are red. > Source/JavaScriptCore/runtime/JSGlobalObject.h:222 > + typedef void (*CompileStreamingPtr)(JSGlobalObject*, ExecState*, JSPromiseDeferred*, JSValue); Use `using`. > Source/JavaScriptCore/runtime/JSGlobalObject.h:225 > + typedef void (*InstantiateStreamingPtr)(JSGlobalObject*, ExecState*, JSPromiseDeferred*, JSValue, JSObject *); Ditto. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:91 > + Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable { > + vm.promiseDeferredTimer->scheduleWorkSoon(promise, [promise, globalObject, result = WTFMove(result), &vm] () mutable { This function is wrong. Its lambda captures promise, importObject, globalObject. But nobody protects them from GC. While webAssemblyCompileFunc protects them with addPendingPromise, webAssemblyModuleValidateAsync does not call it. So it can be collected under the context of webAssemblyModuleValidateAsync. We should call addPendingPromise & dependencies too in this extracted function. And webAssemblyCompileFunc should not call addPendingPromise. I think webAssemblyModuleInstantinateAsyncInternal does that thing. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:214 > +void WebAssemblyPrototype::webAssemblyModuleInstantinateAsyncInternal(ExecState* exec, Vector<uint8_t>&& source, JSObject* importObject, JSPromiseDeferred* promise) Can we define it as a simple global static function in this file scope instead of WebAssemblyPrototype's static function? > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:221 > + Vector<Strong<JSCell>> dependencies; > + dependencies.append(Strong<JSCell>(vm, importObject)); > + vm.promiseDeferredTimer->addPendingPromise(promise, WTFMove(dependencies)); This is really important. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:295 > + JSValue source = exec->argument(0); Drop this line. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:296 > + globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, source); Call `globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, exec->argument(0));` > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:324 > + // Fixme: if there's an importObject and it contains a Memory, then we can compile the module with the right memory type (fast or not) by looking at the memory's type. Use `FIXME: `. And add bugzilla URL for FIXMEs. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:325 > + JSValue source = exec->argument(0); Drop this line. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:326 > + globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, source, importObject); Just calling `globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, exec->argument(0), importObject);` > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h:45 > + static void webAssemblyModuleValidateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSPromiseDeferred*); > + static void webAssemblyModuleInstantinateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSObject*, JSPromiseDeferred*); Why are these two functions are static member functions of WebAssemblyPrototype? Can we define them as static functions in WebAssemblyPrototype.cpp? > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:419 > + handleStreamingAction(globalObject, exec, source, promise, [promise] (JSC::ExecState* exec, const char* data, size_t byteSize) mutable { JSC value `promise` in the lambda is not protected. The lambda does not allow GC to visit its captured values. So it can be collected. Let's protect promise, and dependencies by addPendingPromise. Maybe, promise is protected well in the caller side, but `source` would not be. But personally, if you rewrite some part of compileStreaming/instantiateStreaming in builtin JS, we can just type check this `source` here. And we can extract C++ unwrapped Response. At that time, we can just hold that Ref<>. When calling this function, promise would be already protected by calling addPendingPromise. So if you rewrite the current implementation so, we do not need to call addPendingPromise here. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:420 > + Vector<uint8_t> arrayBuffer; And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:428 > + handleStreamingAction(globalObject, exec, source, promise, [promise, importedObject] (JSC::ExecState* exec, const char* data, size_t byteSize) mutable { Ditto. I believe we should add some assertion that `promise` and `importedObject` is protected by `addPendingPromise` mechanism. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:443 > + JSC::JSFunction* fulfillHandler = JSC::JSNativeStdFunction::create(vm, globalObject, 1, String(), [promise, m_callback = WTFMove(callback), globalObject](JSC::ExecState* fulfillState) { I think we do not need to do this here, I would like to remove JSNativeStdFunction usage as much as I can. And m_callback is not a member variable. I have a bit simple idea: how about resolving promise in JSC's instantiateStreaming etc. function? You define instantiateStreaming etc. as JSC builtin. And inside it, you wrap the given value with the promise. function instantiateStreaming(argument) { return @Promise.@resolve(argument).@then(@instantiateStreamingInternal); } And in JSC's C implementated @instantiateStreamingInternal function, you can just call this hook. And in this hook, we just check JSFetchResponse type instead of handling Promise case. So, you can remove JSNativeStdFunction here. And you can remove this function. instantiateStreaming and compileStreaming just type checks JSFetchResponse and call handleResponseOnStreamingAction. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:469 > + inputResponse->consumeBodyReceivedByChunk([promise, m_callback = WTFMove(callback), globalObject, data = SharedBuffer::create()] (auto&& result) mutable { m_callback is not a right name. It is not a member function. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:498 > + // FIXME: Implement loading for the Blob type Add bugzilla URL.

Comment on attachment 337296 [details] Patch And we should make this behind the flag until `streaming` feature is done. Currently, we are not streaming.

(In reply to Yusuke Suzuki from comment #5) > Comment on attachment 337296 [details] > Patch > > And we should make this behind the flag until `streaming` feature is done. > Currently, we are not streaming. Thanks for review! I'll try to fix during next week :)

Comment on attachment 337296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337296&action=review > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:381 > +static bool tryCopySourceToArrayBuffer(JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, const char* data, size_t byteSize, Vector<uint8_t>&& arrayBuffer) arrayBuffer can't be an rvalue reference, you don't pass it out of the function and the caller tries to reuse it! It should just be normal reference, no move at the callsites. Or you can rename to tryAllocate and return the vector of the right size here, in a std::optional. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:384 > + promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssemlby sources"))); Typo WebAssembly

Comment on attachment 337296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337296&action=review >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:214 >> +void WebAssemblyPrototype::webAssemblyModuleInstantinateAsyncInternal(ExecState* exec, Vector<uint8_t>&& source, JSObject* importObject, JSPromiseDeferred* promise) > > Can we define it as a simple global static function in this file scope instead of WebAssemblyPrototype's static function? Done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:295 >> + JSValue source = exec->argument(0); > > Drop this line. done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:296 >> + globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, source); > > Call `globalObject->globalObjectMethodTable()->compileStreaming(globalObject, exec, promise, exec->argument(0));` Done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:324 >> + // Fixme: if there's an importObject and it contains a Memory, then we can compile the module with the right memory type (fast or not) by looking at the memory's type. > > Use `FIXME: `. And add bugzilla URL for FIXMEs. Done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:326 >> + globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, source, importObject); > > Just calling `globalObject->globalObjectMethodTable()->instantiateStreaming(globalObject, exec, promise, exec->argument(0), importObject);` done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h:45 >> + static void webAssemblyModuleInstantinateAsyncInternal(ExecState*, Vector<uint8_t>&&, JSObject*, JSPromiseDeferred*); > > Why are these two functions are static member functions of WebAssemblyPrototype? Can we define them as static functions in WebAssemblyPrototype.cpp? done >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:381 >> +static bool tryCopySourceToArrayBuffer(JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, const char* data, size_t byteSize, Vector<uint8_t>&& arrayBuffer) > > arrayBuffer can't be an rvalue reference, you don't pass it out of the function and the caller tries to reuse it! It should just be normal reference, no move at the callsites. > > Or you can rename to tryAllocate and return the vector of the right size here, in a std::optional. Done >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:384 >> + promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssemlby sources"))); > > Typo WebAssembly Done >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:443 >> + JSC::JSFunction* fulfillHandler = JSC::JSNativeStdFunction::create(vm, globalObject, 1, String(), [promise, m_callback = WTFMove(callback), globalObject](JSC::ExecState* fulfillState) { > > I think we do not need to do this here, I would like to remove JSNativeStdFunction usage as much as I can. > And m_callback is not a member variable. > > I have a bit simple idea: how about resolving promise in JSC's instantiateStreaming etc. function? > > You define instantiateStreaming etc. as JSC builtin. And inside it, you wrap the given value with the promise. > > function instantiateStreaming(argument) > { > return @Promise.@resolve(argument).@then(@instantiateStreamingInternal); > } > > And in JSC's C implementated @instantiateStreamingInternal function, you can just call this hook. And in this hook, we just check JSFetchResponse type instead of handling Promise case. > So, you can remove JSNativeStdFunction here. And you can remove this function. instantiateStreaming and compileStreaming just type checks JSFetchResponse and call handleResponseOnStreamingAction. Created separated builtin module, WebAssemblyPrototype.js >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:469 >> + inputResponse->consumeBodyReceivedByChunk([promise, m_callback = WTFMove(callback), globalObject, data = SharedBuffer::create()] (auto&& result) mutable { > > m_callback is not a right name. It is not a member function. Renamed >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:498 >> + // FIXME: Implement loading for the Blob type > > Add bugzilla URL. Done

Created attachment 338599 [details] Patch Patch with fixes

Comment on attachment 338599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338599&action=review > Source/JavaScriptCore/runtime/Options.h:487 > + v(bool, useWebAssemblyStreamingApi, true, Normal, "Allow to run WebAssembly's Streaming API") \ Can you add a feature flag the same way as here: https://bugs.webkit.org/show_bug.cgi?id=184312 ? You still have an option, but it's configurable at build time whether it's on or off. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:85 > +static void webAssemblyModuleValidateAsyncInternal(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source) Looks like you want to take ownership of "source" in this function, so it should be an rvalue reference. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:156 > +void WebAssemblyPrototype::webAssemblyModuleValidateAsync(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source) Ditto rvalue ref for "source". > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:316 > + promise->reject(exec, createTypeError(exec, ASCIILiteral("CompileStreaming is not supported in jsc, only in browser environment."))); Is this code even reachable? Seems like it should be an assert instead. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:345 > + promise->reject(exec, createTypeError(exec, ASCIILiteral("InstantiateStreaming is not supported in jsc, only in browser environment."))); Ditto > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:369 > + if (Options::useWebAssemblyStreamingApi()) { Here I think it should only be added then no in the shell as well. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:385 > + promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssembly sources"))); Typo "memory" Also this should be createOutOfMemoryError?

Created attachment 338662 [details] Patch Fix comments

Comment on attachment 338599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338599&action=review >> Source/JavaScriptCore/runtime/Options.h:487 >> + v(bool, useWebAssemblyStreamingApi, true, Normal, "Allow to run WebAssembly's Streaming API") \ > > Can you add a feature flag the same way as here: > https://bugs.webkit.org/show_bug.cgi?id=184312 > ? > > You still have an option, but it's configurable at build time whether it's on or off. In new patch I've introduced ENABLE_WEBASSEMBLY_STREAMING_API feature flag >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:85 >> +static void webAssemblyModuleValidateAsyncInternal(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source) > > Looks like you want to take ownership of "source" in this function, so it should be an rvalue reference. Done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:156 >> +void WebAssemblyPrototype::webAssemblyModuleValidateAsync(ExecState* exec, JSPromiseDeferred* promise, Vector<uint8_t>& source) > > Ditto rvalue ref for "source". Done >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:316 >> + promise->reject(exec, createTypeError(exec, ASCIILiteral("CompileStreaming is not supported in jsc, only in browser environment."))); > > Is this code even reachable? Seems like it should be an assert instead. Fixed >> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:345 >> + promise->reject(exec, createTypeError(exec, ASCIILiteral("InstantiateStreaming is not supported in jsc, only in browser environment."))); > > Ditto Fixed >> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:385 >> + promise->reject(exec, createTypeError(exec, ASCIILiteral("Not enough memmory to load webAssembly sources"))); > > Typo "memory" > > Also this should be createOutOfMemoryError? Fixed

> And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism. @Yusuke Suzuki Do already have function that answer if object is already protected by `addPendingPromise` mechanism, or it has to be introduced in this patch?

Comment on attachment 338662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338662&action=review I have no comments about the functionality of the patch itself, but the changes to the CMake build files are breaking the EWS bots. Please fix at least that and re-upload. > Source/JavaScriptCore/CMakeLists.txt:1122 > + $(JAVASCRIPTCORE_DIR)/builtins/WebAssemblyPrototype.js Please use ${JAVASCRIPTCORE_DIR} here, with brackets, which is how CMake variables have to be expanded. Using parentheses causes CMake to generate invalid output that Ninja cannot parse, and this patch is breaking the EWSs which use Ninja (like GTK+ and WPE).

Created attachment 338861 [details] Patch EWS build

Created attachment 338864 [details] Patch Fix build

Created attachment 338865 [details] Patch Fix build #2

Comment on attachment 338865 [details] Patch Attachment 338865 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7465945 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

Created attachment 338869 [details] Patch Fix build #4

Comment on attachment 338869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338869&action=review r=me with comments. > Source/JavaScriptCore/ChangeLog:73 > + * CMakeLists.txt: > + * Configurations/FeatureDefines.xcconfig: > + * DerivedSources.make: > + * JavaScriptCore.xcodeproj/project.pbxproj: > + * builtins/BuiltinNames.h: > + * builtins/WebAssemblyPrototype.js: Copied from Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.h. > + (compileStreaming): > + (instantiateStreaming): > + * jsc.cpp: > + * runtime/JSGlobalObject.cpp: > + (JSC::JSGlobalObject::init): > + * runtime/JSGlobalObject.h: > + * runtime/Options.h: > + * wasm/js/WebAssemblyPrototype.cpp: > + (JSC::webAssemblyModuleValidateAsyncInternal): > + (JSC::webAssemblyCompileFunc): > + (JSC::WebAssemblyPrototype::webAssemblyModuleValidateAsync): > + (JSC::webAssemblyModuleInstantinateAsyncInternal): > + (JSC::WebAssemblyPrototype::webAssemblyModuleInstantinateAsync): > + (JSC::webAssemblyCompileStreamingInternal): > + (JSC::webAssemblyInstantiateStreamingInternal): > + (JSC::WebAssemblyPrototype::create): > + (JSC::WebAssemblyPrototype::finishCreation): > + * wasm/js/WebAssemblyPrototype.h: Please fix ChangeLog, which includes duplicate entries. > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:357 > +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API; Could you ensure this is disabled right now? (then, we do not have any compileStreaming/instantiateStreaming functions exposed to the Web right now). We should disable this feature until we fully implement streaming functionality. > Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:26 > +function compileStreaming(argument) { Add "use strict"; > Source/JavaScriptCore/builtins/WebAssemblyPrototype.js:30 > +function instantiateStreaming(argument) { Ditto. > Source/JavaScriptCore/runtime/Options.h:540 > + v(enableWebAssemblyStreamingApi, useWebAssemblyStreamingApi, SameOption) \ Remove this. > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:231 > + dependencies.append(Strong<JSCell>(vm, importObject)); Let's add `globalObject` too. > Source/WebCore/Configurations/FeatureDefines.xcconfig:357 > +ENABLE_WEBASSEMBLY_STREAMING_API = ENABLE_WEBASSEMBLY_STREAMING_API; Ditto. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:440 > +void JSDOMWindowBase::handleStreamingAction(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, JSC::JSPromiseDeferred* promise, JSC::JSValue source, std::function<void(JSC::ExecState* exec, const char* data, size_t byteSize)>&& callback) > +{ > + ASSERT(source); > + VM& vm = exec->vm(); > + if (auto inputResponse = JSFetchResponse::toWrapped(vm, source)) > + handleResponseOnStreamingAction(globalObject, exec, inputResponse, promise, WTFMove(callback)); > + else > + promise->reject(exec, createTypeError(exec, ASCIILiteral("first argument must be an Response or Promise for Response"))); > +} This wrapper function seems unnecessary. Just calling handleResponseOnStreamingAction in instantiateStreaming and compileStreaming is simpler. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:442 > +void JSDOMWindowBase::handleResponseOnStreamingAction(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, FetchResponse* inputResponse, JSC::JSPromiseDeferred* promise, std::function<void(JSC::ExecState* exec, const char* data, size_t byteSize)>&& actionCallback) Is it necessary to be a static member function of JSDOMWindowBase? Can we make it per-file static function? And let's use Function<> instead of std::function. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:474 > + auto callback = WTFMove(actionCallback); > + callback(exec, buffer->data(), buffer->size()); `actionCallback(exec, buffer->data(), buffer->size());` should work. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:484 > + auto callback = WTFMove(actionCallback); > + callback(exec, buffer->data(), buffer->size()); Ditto.

(In reply to GSkachkov from comment #13) > > And I believe we should add some assertion that `promise` is protected by `addPendingPromise` mechanism. > > @Yusuke Suzuki Do already have function that answer if object is already > protected by `addPendingPromise` mechanism, or it has to be introduced in > this patch? I think we need to add this in this patch (and calling it in ASSERT). So please add it before landing.

Comment on attachment 338869 [details] Patch Attachment 338869 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7466813 New failing tests: imported/w3c/web-platform-tests/wasm/wasm_stream_instantinate_test.html imported/w3c/web-platform-tests/wasm/wasm_stream_compile_test.html

Created attachment 338870 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

Comment on attachment 338869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338869&action=review > Source/cmake/WebKitFeatures.cmake:177 > + WEBKIT_OPTION_DEFINE(ENABLE_WEBASSEMBLY_STREAMING_API "Toggle WebAssembly streaming api support." PRIVATE ON) The default here should be OFF

Comment on attachment 338869 [details] Patch r=me as well View in context: https://bugs.webkit.org/attachment.cgi?id=338869&action=review > Source/cmake/WebKitFeatures.cmake:177 > + WEBKIT_OPTION_DEFINE(ENABLE_WEBASSEMBLY_STREAMING_API "Toggle WebAssembly streaming api support." PRIVATE ON) The default here should be OFF

Created attachment 339089 [details] Patch Check fixed builds

Created attachment 339090 [details] Patch Rebase & check fixed builds

Created attachment 339093 [details] Patch Check fixed ios builds

Comment on attachment 339093 [details] Patch Attachment 339093 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7500575 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

Created attachment 339098 [details] Patch Check fixed ios-sim&32-bit builds

Comment on attachment 339098 [details] Patch Attachment 339098 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7501852 New failing tests: imported/w3c/web-platform-tests/wasm/wasm_stream_instantiate_test.html imported/w3c/web-platform-tests/wasm/wasm_stream_compile_test.html

Created attachment 339100 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

Created attachment 339146 [details] Patch Check fixed debug build&tests

Patch landed https://trac.webkit.org/r231194

<rdar://problem/39862193>

The iOS Simulator debug build is broken by this change. ./bindings/js/JSDOMWindowBase.cpp:495:1: error: function 'compileStreaming' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn] { ^ ./bindings/js/JSDOMWindowBase.cpp:499:1: error: function 'instantiateStreaming' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn] { ^ https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20%28Build%29/builds/5600

Oh I see. I'll check if we can rollback

(In reply to Ryan Haddad from comment #36) > The iOS Simulator debug build is broken by this change. > > ./bindings/js/JSDOMWindowBase.cpp:495:1: error: function 'compileStreaming' > could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn] > { > ^ > ./bindings/js/JSDOMWindowBase.cpp:499:1: error: function > 'instantiateStreaming' could be declared with attribute 'noreturn' > [-Werror,-Wmissing-noreturn] > { > ^ > > https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Debug%20%28Build%29/builds/5600 I seems that we already faced with this issue https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/jit/JITArithmetic32_64.cpp#L347 At least in this code, assertion was commented out. I don't know what to do, or rollback patch, or just create new small patch with commented ASSERT_NOT_REACHED

Comment on attachment 339146 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.