Bug 170612 - WebAssembly: Module::getOrCreateCodeBlock is wrong
Summary: WebAssembly: Module::getOrCreateCodeBlock is wrong
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:
Depends on:
Blocks:
 
Reported: 2017-04-07 12:07 PDT by Saam Barati
Modified: 2017-04-07 12:43 PDT (History)
10 users (show)

See Also:

Attachments
patch (2.69 KB, patch)
2017-04-07 12:14 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (1.74 KB, patch)
2017-04-07 12:33 PDT, Saam Barati 		keith_miller: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-04-07 12:07:49 PDT 
It needs to check something that isn't runnable()
Comment 1 Saam Barati 2017-04-07 12:14:09 PDT 
Created attachment 306526 [details]
patch
Comment 2 JF Bastien 2017-04-07 12:20:21 PDT 
Comment on attachment 306526 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306526&action=review

> Source/JavaScriptCore/wasm/WasmCodeBlock.h:65
> +            return false;

"don't know" shouldn't be false? Can you use an optional instead?
Comment 3 Keith Miller 2017-04-07 12:21:30 PDT 
Comment on attachment 306526 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306526&action=review

>> Source/JavaScriptCore/wasm/WasmCodeBlock.h:65
>> +            return false;
> 
> "don't know" shouldn't be false? Can you use an optional instead?

I think we should just use a TriState.
Comment 4 Saam Barati 2017-04-07 12:24:38 PDT 
Comment on attachment 306526 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306526&action=review

>>> Source/JavaScriptCore/wasm/WasmCodeBlock.h:65
>>> +            return false;
>> 
>> "don't know" shouldn't be false? Can you use an optional instead?
> 
> I think we should just use a TriState.

Maybe I'll just make the caller do this. Since we just have one caller.
if (!codeBlock || (codeBlock->compilationFinished() && !codeBlock->runnable())
Comment 5 Saam Barati 2017-04-07 12:33:19 PDT 
Created attachment 306527 [details]
patch
Comment 6 Keith Miller 2017-04-07 12:36:07 PDT 
Comment on attachment 306527 [details]
patch

r=me.
Comment 7 Saam Barati 2017-04-07 12:43:16 PDT 
landed in:
https://trac.webkit.org/changeset/215114/webkit