[webkit-reviews] review granted: [Bug 194637] testb3 and testair should test O0/O1/O2 : [Attachment 362242] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 194637: testb3 and testair should test O0/O1/O2
https://bugs.webkit.org/show_bug.cgi?id=194637

Attachment 362242: patch

https://bugs.webkit.org/attachment.cgi?id=362242&action=review




--- Comment #8 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 362242
  --> https://bugs.webkit.org/attachment.cgi?id=362242
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::.