This is the first step towards simulating potential JS exceptions throws, and verifying that they are checked for appropriately in C++ code.
Created attachment 286989 [details] work in progress
Comment on attachment 286989 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=286989&action=review > Source/JavaScriptCore/runtime/ThrowScope.h:39 > + > +#ifndef ENABLE_EXCEPTION_CHECK_VERIFICATION > +#define ENABLE_EXCEPTION_CHECK_VERIFICATION (!defined(NDEBUG)) > +#endif Let's call this ENABLE_THROW_SCOPE_VERIFICATION. Let's put this setting in Platform.h with other settings. > Source/JavaScriptCore/runtime/ThrowScope.h:76 > +#define MAKE_THROW_SCOPE(name__, vm__) \ Let's call this DECLARE_THROW_SCOPE. Instead of taking the local variable name as a parameter, let's do this: auto scope = DECLARE_THROW_SCOPE(vm). That way, even though the declaration is a macro mystery, the nature of the local variable is not.
Created attachment 287336 [details] x86_64 benchmark result run 1
Created attachment 287337 [details] x86_64 benchmark result run 2
There's some regressions and progressions in the microbenchmarks. All other so-called "definitely"s are not consistently reproducible on different runs. A inspection of disassembled code for regressing benchmarks did not seem to show any bad code that can account for the regressions. It looks to me like the regressions are just be due to cache effects. I'll upload the updated patch shortly for review.
Created attachment 287341 [details] proposed patch.
Comment on attachment 287341 [details] proposed patch. I ran the API tests, but forgot to run the bindings test. I need to rebase the results there. Will upload a new patch shortly.
Created attachment 287349 [details] proposed patch.
Comment on attachment 287349 [details] proposed patch. Attachment 287349 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1968587 Number of test failures exceeded the failure limit.
Created attachment 287359 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287349 [details] proposed patch. I had some silly bugs that resulted in the failures. Will fix first.
Created attachment 287375 [details] proposed patch.
Created attachment 287378 [details] proposed patch.
Comment on attachment 287378 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=287378&action=review > Source/JavaScriptCore/runtime/ThrowScope.cpp:297 > + if (topCallFrame && previousScope > topCallFrame) > + willBeHandleByLLIntOrJIT = true; Is it clear that it's better to have this here, than to have logic in vmEntry? > Source/JavaScriptCore/runtime/ThrowScope.h:42 > +class ThrowScope { > +public: > + JS_EXPORT_PRIVATE ThrowScope(VM&, ThrowScopeLocation); > + JS_EXPORT_PRIVATE ~ThrowScope(); > + Seems like you could delete the copy constructor/operator but enable the move constructor/operator. I think you would do for example: ThrowScope(const ThrowScope&) = delete; ThrowScope(ThrowScope&&) = default; Clearly, ThrowScope is not meant to be copied, so this protects against that. But it is meant to be created by moving, via that macro. I believe this should disallow the bad thing (copying) but allow the good thing (your auto scope = DECLARE_blah idiom). > Source/JavaScriptCore/runtime/ThrowScope.h:77 > + JSC::ThrowScope(vm__, JSC::ThrowScopeLocation(__FUNCTION__, __FILE__, __LINE__)) Put extra () around vm__, like "(vm__)". This makes this macro behave more like a function, in case there are for example commas in vm__.
Comment on attachment 287378 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=287378&action=review >> Source/JavaScriptCore/runtime/ThrowScope.cpp:297 >> + willBeHandleByLLIntOrJIT = true; > > Is it clear that it's better to have this here, than to have logic in vmEntry? VMEntry (in the comment above) is a misnomer on my part. I meant to communicate first ThrowScope after a JS frame on the stack. "VMEntry" is the wrong idea to communicate this. Rereading the comment, I think just removing the "VMEntry" would do the trick. The scenario we need to guard against is where a JS function calls a C++ operation helper, and the helper has a ThrowScope. We need to clear the needExceptionCheck flag before returning to the JS frame because the JS code may invoke another C++ helper (and fail verification on entry to that new ThrowScope). There is no VMEntryScope involved here. >> Source/JavaScriptCore/runtime/ThrowScope.h:42 >> + > > Seems like you could delete the copy constructor/operator but enable the move constructor/operator. I think you would do for example: > > ThrowScope(const ThrowScope&) = delete; > ThrowScope(ThrowScope&&) = default; > > Clearly, ThrowScope is not meant to be copied, so this protects against that. But it is meant to be created by moving, via that macro. I believe this should disallow the bad thing (copying) but allow the good thing (your auto scope = DECLARE_blah idiom). Great. I will apply and test. >> Source/JavaScriptCore/runtime/ThrowScope.h:77 >> + JSC::ThrowScope(vm__, JSC::ThrowScopeLocation(__FUNCTION__, __FILE__, __LINE__)) > > Put extra () around vm__, like "(vm__)". This makes this macro behave more like a function, in case there are for example commas in vm__. Will do.
I'm now trying to figure out why the Win EWS is having a bad time.
Comment on attachment 287378 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=287378&action=review > Source/JavaScriptCore/runtime/ThrowScope.cpp:296 > + if (topCallFrame && previousScope > topCallFrame) Since we don't clear VM::m_topCallFrame most of the time, topCallFrame will almost always be non-null, and we will get away with a lot of mistakes.
Comment on attachment 287378 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=287378&action=review >> Source/JavaScriptCore/runtime/ThrowScope.cpp:296 >> + if (topCallFrame && previousScope > topCallFrame) > > Since we don't clear VM::m_topCallFrame most of the time, topCallFrame will almost always be non-null, and we will get away with a lot of mistakes. ...Actually, even though we leave stale values for VM::topCallFrame during LLInt and JIT execution, once we exit, we restore null.
I think you can fix the Windows build failure by using old-school header guards instead of #pragma once. Later, we should get DumpRenderTree.cpp not to include JavaScriptCore/runtime headers. It doesn't need to, since we have typed array API now. Phil, any further comments?
Created attachment 287410 [details] revised patch w/ Fil's feedback + old header guard applied.
Thanks for the reviews. Landed in r205198: <http://trac.webkit.org/r205198>.