...
Created attachment 362096 [details] WIP
Created attachment 362242 [details] patch
Comment on attachment 362242 [details] patch Attachment 362242 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11184355 New failing tests: js/dom/custom-constructors.html
Created attachment 362253 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 362242 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362242&action=review LGTM > Source/JavaScriptCore/b3/testb3.cpp:-3537 > - root->appendNew<Const32Value>(proc, Origin(), -1), Please add a comment in the changelog about this (maybe with a link to the other issue you opened about it?) > Source/JavaScriptCore/b3/testb3.cpp:13885 > + else { Is there a reason not to do "else if" on one line and skip the braces? > Source/JavaScriptCore/b3/testb3.cpp:14011 > + Probably some accidental space. > Source/JavaScriptCore/b3/testb3.cpp:16549 > + dataLog(PREFIX #test "...\n"); \ I like the PREFIX macro. Can you move the '\' back to its column? It is a bit weird for it to be aligned through the rest of this block but not this line. > Source/JavaScriptCore/b3/testb3.cpp:16551 > + dataLog(PREFIX #test ": OK!\n"); \ ditto. > Source/JavaScriptCore/b3/testb3.cpp:18161 > crashLock.lock(); What is this crashLock.lock()/unlock() for? Is it making sure that no one is printing an error message before exiting? In any case, I don't think the unlock() is useful: it is automatically destructed at the end of the function.
Comment on attachment 362242 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362242&action=review >> Source/JavaScriptCore/b3/testb3.cpp:16549 >> + dataLog(PREFIX #test "...\n"); \ > > I like the PREFIX macro. Can you move the '\' back to its column? It is a bit weird for it to be aligned through the rest of this block but not this line. Will fix. >> Source/JavaScriptCore/b3/testb3.cpp:18161 >> crashLock.lock(); > > What is this crashLock.lock()/unlock() for? Is it making sure that no one is printing an error message before exiting? > In any case, I don't think the unlock() is useful: it is automatically destructed at the end of the function. It is not. It's a global variable.
> >> Source/JavaScriptCore/b3/testb3.cpp:18161 > >> crashLock.lock(); > > > > What is this crashLock.lock()/unlock() for? Is it making sure that no one is printing an error message before exiting? > > In any case, I don't think the unlock() is useful: it is automatically destructed at the end of the function. > > It is not. It's a global variable. Oh right, I had missed that. I just saw all the unpaired calls to crashLock.lock() and assumed it was a local thing, but these are only fine because they are followed by CRASH(). Then I've got nothing else to suggest, but I cannot r+ as I am not a reviewer yet.
Comment on attachment 362242 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362242&action=review r=me with fixes. >> Source/JavaScriptCore/b3/testb3.cpp:-3537 >> - root->appendNew<Const32Value>(proc, Origin(), -1), > > Please add a comment in the changelog about this (maybe with a link to the other issue you opened about it?) I think this is worth a comment in the ChangeLog as to why this is needed. >>> Source/JavaScriptCore/b3/testb3.cpp:18161 >>> crashLock.lock(); >> >> What is this crashLock.lock()/unlock() for? Is it making sure that no one is printing an error message before exiting? >> In any case, I don't think the unlock() is useful: it is automatically destructed at the end of the function. > > It is not. It's a global variable. @Robin, the unlock is necessary because we will now run the run() function multiple times. If we do not unlock it, we'll deadlock on the next pass when we get to the lock() above this. > Source/JavaScriptCore/b3/testb3.cpp:18193 > + Options::defaultB3OptLevel() = i; Qualifying Options::defaultB3OptLevel() with JSC:: should fix all the 32-bit build failures. > Source/JavaScriptCore/b3/air/testair.cpp:2212 > + Options::defaultB3OptLevel() = i; Prefix Options::... with JSC::.
Comment on attachment 362242 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=362242&action=review >>> Source/JavaScriptCore/b3/testb3.cpp:-3537 >>> - root->appendNew<Const32Value>(proc, Origin(), -1), >> >> Please add a comment in the changelog about this (maybe with a link to the other issue you opened about it?) > > I think this is worth a comment in the ChangeLog as to why this is needed. Robin already changed this in a previous patch.
landed in: https://trac.webkit.org/changeset/242068/webkit
<rdar://problem/48388097>