Patch coming.
Created attachment 295213 [details] proposed patch.
Created attachment 295294 [details] proposed patch with more fixes.
Comment on attachment 295294 [details] proposed patch with more fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=295294&action=review > Source/JavaScriptCore/jsc.cpp:385 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Too late since we already have so may of these, but I really prefer to use "{ }" in cases like this one rather than "encodedJSValue()". > Source/JavaScriptCore/jsc.cpp:1443 > + RELEASE_ASSERT(!scope.exception()); I saw the comment about out of memory, but is this really the right thing to do? This is a command line tool, so shouldn't we write something out to stderr and then exit instead of aborting? Seems more elegant to me. In all the places where the code now says RELEASE_ASSERT. > Source/JavaScriptCore/jsc.cpp:2508 > + RETURN_IF_EXCEPTION(scope, JSValue()); Same thing would work here, using { } instead of JSValue(). In fact, if we made a version of the RETURN_IF_EXCEPTION macro that used { }, we could use it in functions that return JSValue, EncodedJSValue, and any pointer type. We should really do that. The only place we couldn’t use it is in functions that return void, I think.
Comment on attachment 295294 [details] proposed patch with more fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=295294&action=review >> Source/JavaScriptCore/jsc.cpp:1443 >> + RELEASE_ASSERT(!scope.exception()); > > I saw the comment about out of memory, but is this really the right thing to do? This is a command line tool, so shouldn't we write something out to stderr and then exit instead of aborting? Seems more elegant to me. In all the places where the code now says RELEASE_ASSERT. I'll defer landing this patch until I can do more analysis on this issue, and see if we can do better. >> Source/JavaScriptCore/jsc.cpp:2508 >> + RETURN_IF_EXCEPTION(scope, JSValue()); > > Same thing would work here, using { } instead of JSValue(). > > In fact, if we made a version of the RETURN_IF_EXCEPTION macro that used { }, we could use it in functions that return JSValue, EncodedJSValue, and any pointer type. We should really do that. The only place we couldn’t use it is in functions that return void, I think. I'll change these to use { }. We had entertained having different versions of the RETURN_IF_EXCEPTION() macro before for different return types (2 at the time), but opted to go with just 1 macro for consistency. If we go with a special macro for returning { }, then we'll end up with 3 macros: 1 for void, 1 for { }, and 1 for anything else (there are a few places where we need that). I'll stick with the 1 RETURN_IF_EXCEPTION() macro that we have currently for now.
Created attachment 295302 [details] Updated patch. Still need to look into the use of RELEASE_ASSERTs, and see if we can do better.
Comment on attachment 295302 [details] Updated patch. It is invalid to replace returning encodedJSValue() with returning { }. On 32-bit builds, the former is non-zero, while the latter is 0. Will fix this patch.
Created attachment 295418 [details] proposed patch.
Comment on attachment 295418 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295418&action=review > Source/JavaScriptCore/jsc.cpp:2510 > JSString* type = jsCast<JSString*>(wasmValue.get(exec, makeIdentifier(vm, "type"))); > + RETURN_IF_EXCEPTION(scope, { }); > JSValue value = wasmValue.get(exec, makeIdentifier(vm, "value")); > + RETURN_IF_EXCEPTION(scope, { }); I'd make these assertions since they're for testing WASM
Comment on attachment 295418 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295418&action=review >> Source/JavaScriptCore/jsc.cpp:2510 >> + RETURN_IF_EXCEPTION(scope, { }); > > I'd make these assertions since they're for testing WASM I've applied this change. > Source/JavaScriptCore/jsc.cpp:2590 > + for (unsigned argIndex = 0; argIndex < arguments->length(); ++argIndex) { > + JSValue boxedValue = box(exec, vm, arguments->getIndexQuickly(argIndex)); > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + boxedArgs.append(boxedValue); > + } Since we've changed box() to never throw but to assert no exceptions instead, this change is now unnecessary. > Source/JavaScriptCore/jsc.cpp:2594 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Ditto. This can be removed too.
Created attachment 304546 [details] re-based patch for landing.
Created attachment 304553 [details] re-based patch for landing: w/ build fix for release builds.
Thanks for the reviews. Landed in r214016: <http://trac.webkit.org/r214016>.