Bug 170135 – WebAssembly: JSWebAssemblyCallee should not be a JSCell

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

[Saam Barati]

This will make PIC easier.
However, in the future, when we move to a baseline compilation, we need to find a way to throw away baseline code while it's executing in parallel in multiple workers.

[JF Bastien]

I think this is done in bug #169773?

[Saam Barati]

(In reply to JF Bastien from comment #1)
> I think this is done in bug #169773?

That patch does not make callee not a JSCell.

[Saam Barati]

Created attachment 305542 [details]
WIP

Doesn't compile yet

[Saam Barati]

I'm also decoupling CCallHelpers/AssemblyHelpers from a VM in this patch. Any function that does VM-y things, will take a VM& as a parameter.

[Saam Barati]

Created attachment 305549 [details]
WIP

Still doesn't compile

[Saam Barati]

Created attachment 305555 [details]
WIP

[Saam Barati]

I think everything else is in place for this to happen.

[Saam Barati]

Created attachment 306124 [details]
WIP

It's still crashing on some wasm tests.
I'm re-writing the various runtime bits that wasm uses to make sure they don't use exec->vm() because that assumes we have a JSCell Callee

[Saam Barati]

Created attachment 306143 [details]
WIP

passes wasm tests.

[Saam Barati]

Created attachment 306152 [details]
WIP

Catching an exception w/ web inspector open no longer crashes. But it still doesn't show any wasm frames. We should fix that soon.

[Saam Barati]

Created attachment 306157 [details]
WIP

Almost done, just need to rename some files.

[Saam Barati]

Created attachment 306164 [details]
patch

[Build Bot]

Attachment 306164 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:190:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/CalleeBits.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/wasm/WasmPlan.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/wasm/WasmContext.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 306164 [details]
patch

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

Some tests are sad. Looks good overall, thought I'm not familiar with some of the code so it would be good to have another set of eyes looking.

> Source/JavaScriptCore/jsc.cpp:3206
> +    Vector<Ref<Wasm::Callee>> keepAlive;

It's not obvious what the difference is between these two from just their name. I know it's pre-existing, but still confusing IMO.

> Source/JavaScriptCore/wasm/WasmCallee.cpp:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2017

> Source/JavaScriptCore/wasm/WasmCallee.h:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2017

> Source/JavaScriptCore/wasm/WasmPlanInlines.h:43
> +            return bitwise_cast<void*>(bitwise_cast<uintptr_t>(callee) | 1);

Could you keep the | 1 with the same code that does & ~1 ? That would prevent it from ever diverging, or being missed in a copy-paste tragedy.

> Source/JavaScriptCore/wasm/WasmThunks.cpp:76
> +        // this since it is compiled with a constant VM pointer.) We could make the calling convetion

Typo "convention"

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:79
> +    // These two callee getters are only valid once the fields have been polulated.

Typo "populated"

> Source/JavaScriptCore/wasm/js/JSWebAssemblyRuntimeError.cpp:35
> +const ClassInfo JSWebAssemblyRuntimeError::s_info = { "WebAssembly.RuntimeError", &Base::s_info, 0, CREATE_METHOD_TABLE(JSWebAssemblyRuntimeError) };

The other two wasm error types were auto-generated together and should have the same position as here. I'd rather update all or none.

[Michael Saboff]

Looks like there are some 32 bit build errors.

[Saam Barati]

Comment on attachment 306164 [details]
patch

Will work on 32-bit

[Saam Barati]

Comment on attachment 306164 [details]
patch

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

>> Source/JavaScriptCore/jsc.cpp:3206
>> +    Vector<Ref<Wasm::Callee>> keepAlive;
> 
> It's not obvious what the difference is between these two from just their name. I know it's pre-existing, but still confusing IMO.

Ok. I'll name them better.

>> Source/JavaScriptCore/wasm/WasmCallee.h:2
>> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
> 
> 2017

Why? This file was "svn mv"ed.

>> Source/JavaScriptCore/wasm/WasmPlanInlines.h:43
>> +            return bitwise_cast<void*>(bitwise_cast<uintptr_t>(callee) | 1);
> 
> Could you keep the | 1 with the same code that does & ~1 ? That would prevent it from ever diverging, or being missed in a copy-paste tragedy.

Yeah definitely. Will fix this.

[Saam Barati]

Created attachment 306184 [details]
patch

[Build Bot]

Attachment 306184 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CallFrame.cpp:190:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/CalleeBits.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 306184 [details]
patch

r=me

[Saam Barati]

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