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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 25 17:22:12 PST 2019

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

Attachment 362242: patch


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

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::.

More information about the webkit-reviews mailing list