Bug 194637 - testb3 and testair should test O0/O1/O2
Summary: testb3 and testair should test O0/O1/O2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-13 20:09 PST by Saam Barati
Modified: 2019-02-25 19:20 PST (History)
14 users (show)

See Also:


Attachments
WIP (25.36 KB, patch)
2019-02-14 22:41 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (36.72 KB, patch)
2019-02-17 12:41 PST, Saam Barati
mark.lam: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.87 MB, application/zip)
2019-02-17 14:23 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-02-13 20:09:45 PST
...
Comment 1 Saam Barati 2019-02-14 22:41:21 PST
Created attachment 362096 [details]
WIP
Comment 2 Saam Barati 2019-02-17 12:41:39 PST
Created attachment 362242 [details]
patch
Comment 3 Build Bot 2019-02-17 14:23:45 PST
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
Comment 4 Build Bot 2019-02-17 14:23:57 PST
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 5 Robin Morisset 2019-02-18 10:15:48 PST
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 6 Saam Barati 2019-02-18 11:27:15 PST
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.
Comment 7 Robin Morisset 2019-02-18 13:26:42 PST
> >> 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 8 Mark Lam 2019-02-25 17:22:12 PST
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 9 Saam Barati 2019-02-25 19:07:02 PST
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.
Comment 10 Saam Barati 2019-02-25 19:16:58 PST
landed in:
https://trac.webkit.org/changeset/242068/webkit
Comment 11 Radar WebKit Bug Importer 2019-02-25 19:20:57 PST
<rdar://problem/48388097>