Bug 194637 – testb3 and testair should test O0/O1/O2

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 362096 [details]
WIP

[Saam Barati]

Created attachment 362242 [details]
patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Robin Morisset]

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.

[Saam Barati]

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.

[Robin Morisset]

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

[Mark Lam]

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

[Saam Barati]

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.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/242068/webkit

[Radar WebKit Bug Importer]

<rdar://problem/48388097>