Bug 164133 - WebAssembly JS API: implement Global
Summary: WebAssembly JS API: implement Global
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks: 161709 165700
  Show dependency treegraph
 
Reported: 2016-10-28 10:12 PDT by JF Bastien
Modified: 2016-12-14 19:37 PST (History)
6 users (show)

See Also:


Attachments
WIP (26.73 KB, text/plain)
2016-12-09 10:04 PST, Keith Miller
no flags Details
Patch (61.90 KB, patch)
2016-12-13 16:29 PST, Keith Miller
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-10-28 10:12:43 PDT
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#global-section

Also allow importing / exporting them.
Comment 1 Keith Miller 2016-12-09 10:04:25 PST
Created attachment 296655 [details]
WIP
Comment 2 Keith Miller 2016-12-13 16:29:08 PST
Created attachment 297048 [details]
Patch
Comment 3 WebKit Commit Bot 2016-12-13 16:30:21 PST
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Comment 4 Saam Barati 2016-12-13 17:17:41 PST
Comment on attachment 297048 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/wasm/WasmFormat.h:113
> +        Mutable = 1,
> +        Immutable = 0

Probably worth a comment saying this is to match the binary format.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:269
> +            if (!parseGlobalType(global))
> +                return false;

Shouldn't this return false if the global is Mutable? I think you also need a test for this.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:448
> +        switch (initializerOpcode) {

Should this be a helper since its parsing init_expr (or whatever it's called) and it'll be used from elsewhere?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
> +    m_errorMessage = ASCIILiteral("Attempt to use unknown global.");

nit: I think this should be "Attempted"
Also, this needs a test.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:204
> +        m_errorMessage = ASCIILiteral("Attempt to use unknown global.");

Ditto style and also needs a test.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:209
> +        m_errorMessage = ASCIILiteral("Attempt to store to immutable global.");

Ditto style and also needs a test.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:218
> +    m_errorMessage = makeString("Attempt to set global with type: ", toString(globalType), " with a variable of type: ", toString(value));

You should add a test for this.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:445
> +    Validate context(signature->returnType, info.globals, info.memory);

Style: Maybe just pass in "info" instead since we're now using two fields on it?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:43
> +    // FIXME: These objects çould be pretty big we should try to throw OOM here.

Weird unicode here?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:45
> +    instance->m_globals = reinterpret_cast<GlobalDataDescriptor*>(fastMalloc(info.globals.size() * sizeof(Register)));

Style: I'd make this a MallocPtr.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:73
> +    fastFree(m_globals);

And then you can get rid of implementing the destructor.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:91
> +    visitor.addOpaqueRoot(thisObject->m_globals);

Why? This looks wrong.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:145
> +

Please remove this newline.

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:200
> +            if (!value.isNumber())

You should add a test for this.

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:215
> +            switch (moduleInformation.globals[import.kindIndex].type) {
> +            case Wasm::I32:
> +                instance->setGlobal(numImportGlobals++, value.toInt32(exec));
> +                break;
> +            case Wasm::F32:
> +                instance->setGlobal(numImportGlobals++, bitwise_cast<uint32_t>(value.toFloat(exec)));
> +                break;
> +            case Wasm::F64:
> +                instance->setGlobal(numImportGlobals++, bitwise_cast<uint64_t>(value.asNumber()));
> +                break;
> +            default:
> +                RELEASE_ASSERT_NOT_REACHED();
> +            }

You need to add an ASSERT(!throwScope.exception()) after this switch

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:265
> +                instance->setGlobal(globalIndex, instance->loadI64Global(global.initialBitsOrImportNumber));

Why i64? Just to get the bits?

> JSTests/wasm/Builder.js:526
> +                        throw new Error(`Start sections function index  must either be a number or a string`);

Why?
Comment 5 JF Bastien 2016-12-13 17:49:40 PST
Comment on attachment 297048 [details]
Patch

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

You're missing one of the json files.

>> Source/JavaScriptCore/wasm/WasmFormat.h:113
>> +        Immutable = 0
> 
> Probably worth a comment saying this is to match the binary format.

Or... wait for it... AUTOGEN!!! 🎉

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:102
> +        m_globals[i].i64 = bits;

You should use memcpy to avoid aliasing problems (memcpy makes all union members active).

> JSTests/wasm/Builder_WebAssemblyBinary.js:63
> +        throw new Error(`mutability should be either "mutable" or "immutable", but got ${global.mutablity}`);

This should be done in the Builder, and an invalid integer should be allowed in unchecked mode. At this point you should only have an integer.
Comment 6 Keith Miller 2016-12-14 12:46:25 PST
Comment on attachment 297048 [details]
Patch

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

>>> Source/JavaScriptCore/wasm/WasmFormat.h:113
>>> +        Immutable = 0
>> 
>> Probably worth a comment saying this is to match the binary format.
> 
> Or... wait for it... AUTOGEN!!! 🎉

Added a FIXME pointing to a bug.

>> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:269
>> +                return false;
> 
> Shouldn't this return false if the global is Mutable? I think you also need a test for this.

Yes. Test added.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:197
>> +    m_errorMessage = ASCIILiteral("Attempt to use unknown global.");
> 
> nit: I think this should be "Attempted"
> Also, this needs a test.

We use some combination of "Attempting" and "Attempt" everywhere else. If we want to make the wording more consistent we should do so in a follow up patch.
Added test.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:204
>> +        m_errorMessage = ASCIILiteral("Attempt to use unknown global.");
> 
> Ditto style and also needs a test.

ditto.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:209
>> +        m_errorMessage = ASCIILiteral("Attempt to store to immutable global.");
> 
> Ditto style and also needs a test.

ditto.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:445
>> +    Validate context(signature->returnType, info.globals, info.memory);
> 
> Style: Maybe just pass in "info" instead since we're now using two fields on it?

done. I also renamed info to module since that's a better name anyway.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:45
>> +    instance->m_globals = reinterpret_cast<GlobalDataDescriptor*>(fastMalloc(info.globals.size() * sizeof(Register)));
> 
> Style: I'd make this a MallocPtr.

Changed.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:91
>> +    visitor.addOpaqueRoot(thisObject->m_globals);
> 
> Why? This looks wrong.

This was supposed to be reportExtraMemoryVisited.

>> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:265
>> +                instance->setGlobal(globalIndex, instance->loadI64Global(global.initialBitsOrImportNumber));
> 
> Why i64? Just to get the bits?

Yeah.
Comment 7 Keith Miller 2016-12-14 13:29:51 PST
Committed r209830: <http://trac.webkit.org/changeset/209830>
Comment 8 Keith Miller 2016-12-14 19:37:04 PST
rdar://problem/29599104