1244405 - Baldr: add memory section

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

This bug just adds the memory section; load/store to access memory will be separate.

Comment 1

[Luke Wagner [:luke]]

Attachment: memory-section (WIP)

Needs tests, comments and to be split up into patches.

Comment 2

[Luke Wagner [:luke]]

Attachment: move-constants

This patch moves some constants and static asserts out of AsmJS.* and renames some heap constants from AsmJS* to Wasm*.

Comment 3

[Luke Wagner [:luke]]

Attachment: wasm-buffer

This patch factors an ArrayBuffer::createForWasm used in the next patch.

Comment 4

[Luke Wagner [:luke]]

Attachment: memory-section

Pretty easy, given preceding.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-export-name

This patch makes generated exported function names match the names generated for stack-walking.  Doing this led to factoring out the building of ExportMap from AsmJS/Wasm.cpp into ModuleGenerator which simplifies the code a bit.  Technically unrelated but prepares for the next patch which adds a second kind of export (memory).

Comment 6

[Luke Wagner [:luke]]

Attachment: memory-export

This adds '(export "foo" memory)' as a second kind of export which has a nice symmetry with normal function imports.

Comment 7

[Luke Wagner [:luke]]

Attachment: fix-export-name

Actually, the 'DynamicLinkData' was not that useful of an abstraction when paired with the change in this patch and the change in the next patch, so it is removed.

Comment 8

[Luke Wagner [:luke]]

Attachment: memory-section

This version creates the buffer when decoding the memory section. This will be useful when adding data segments since it will allow the data segments to be copied directly into the buffer, avoiding an intermediate copy of potentially large segments.

Comment 9

[Luke Wagner [:luke]]

Attachment: memory-export

Rebased.

Comment 10

[Luke Wagner [:luke]]

Attachment: memory-segments

Last patch for this bug adds (segment) for initializing memory.  Spreading the reviews around a bit...

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8715031 [details] [diff] [review]
memory-segments

Review of attachment 8715031 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmText.cpp
@@ +593,5 @@
>      return c == '_' || IsWasmDigit(c) || IsWasmLetter(c);
>  }
>  
> +static bool
> +IsHexDigit(char c, uint8_t* value)

value can be a pointer-to-const.

@@ +595,5 @@
>  
> +static bool
> +IsHexDigit(char c, uint8_t* value)
> +{
> +    c = tolower(c);

tolower is locale-sensitive, which isn't desirable here. Just as we have our own IsWasmLetter etc., we should do our own thing here.

@@ +631,5 @@
> +    switch (*cur) {
> +      case 'n': u8 = '\n'; break;
> +      case 't': u8 = '\t'; break;
> +      case '\\': u8 = '\\'; break;
> +      case '\"': u8 = '\"'; break;

The spec repo also recognizes '\'' in this context.

@@ +2075,5 @@
> +        MOZ_ALWAYS_TRUE(ConsumeTextByte(&cur, end, &byte));
> +        bytes.infallibleAppend(byte);
> +    }
> +
> +    if (!e.writeVarU32(bytes.length()))

This implicitly converts bytes.length() to uint32_t, but I'm not currently able to find any code which limits the size of the data held in bytes to the uint32_t range. Does this need a length check? Or if it doesn't, an assert here would be good.

@@ +2099,5 @@
> +    size_t offset;
> +    if (!e.startSection(&offset))
> +        return false;
> +
> +    if (!e.writeVarU32(segments.length()))

Similarly, this converts to uint32_t, and the number of segments doesn't appear explicitly bounded.

Comment 12

[Dan Gohman [:sunfish]]

Comment on attachment 8715031 [details] [diff] [review]
memory-segments

Review of attachment 8715031 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmText.cpp
@@ +593,5 @@
>      return c == '_' || IsWasmDigit(c) || IsWasmLetter(c);
>  }
>  
> +static bool
> +IsHexDigit(char c, uint8_t* value)

Oops, I misread the code here. Ignore this comment :).

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Dan Gohman [:sunfish] from comment #11)
> > +static bool
> > +IsHexDigit(char c, uint8_t* value)
> > +{
> > +    c = tolower(c);
> 
> tolower is locale-sensitive, which isn't desirable here.

And...thread-unsafe!  

Comment 14

[Luke Wagner [:luke]]

Hah, and here I thought I was just saving myself an extra if-then.  I'll just add an explicit 'A'-'F' case :)

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> And...thread-unsafe!

Let the record reflect that my comment as I wrote it had a U+1F4AF HUNDRED POINTS SYMBOL after this, that Bugzilla ate.  :-)  Known bug in Bugzilla, on file.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714359 [details] [diff] [review]
move-constants

Review of attachment 8714359 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand how you choose to replace AsmJSPageSize by either wasm::PageSize or gc::SystemPageSize(). Can you please explain this?

::: js/src/asmjs/AsmJS.cpp
@@ +8267,5 @@
>      // See EstablishPreconditions.
>  #if defined(JS_CODEGEN_NONE) || defined(JS_CODEGEN_ARM64)
>      bool available = false;
>  #else
> +    bool available = HasCompilerSupport(cx) &&

You can remove the #ifdef because HasCompilerSupport will return false for NONE and ARM64 \o/

::: js/src/asmjs/Wasm.h
@@ +33,5 @@
> +HasCompilerSupport(ExclusiveContext* cx);
> +
> +// The WebAssembly spec hard-codes the virtual page size to be 64KiB and limits
> +// forces the linear memory to always be a multiple of 64KiB.
> +static const unsigned PageSize = 64 * 1024;

As this is larger than the requirements for both MIPS platforms, I assume this won't be an issue for the MIPS platforms?

@@ +40,5 @@
> +// reserved and the subrange [0, memory_size) is given readwrite permission.
> +// See also static asserts in MIRGenerator::foldableOffsetRange.
> +#ifdef ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB
> +static const uint64_t Uint32Range = uint64_t(UINT32_MAX) + 1;
> +static const uint64_t MappedSize = 2 * Uint32Range + PageSize;

In the previous version, it was Uint32Range + constants, now 2 * Uint32Range + constants. Is that intended?

::: js/src/vm/ArrayBufferObject.cpp
@@ +434,5 @@
>      }
>  #   if defined(MOZ_VALGRIND) && defined(VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE)
>      // Tell Valgrind/Memcheck to not report accesses in the inaccessible region.
>      VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE((unsigned char*)data + validLength,
> +                                                   wasm::MappedSize-validLength);

nit: pre-existing: can you add spaces before/after the minus operator, please?

Comment 17

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
> I don't understand how you choose to replace AsmJSPageSize by either
> wasm::PageSize or gc::SystemPageSize(). Can you please explain this?

The WebAssembly defines a virtual page size (64KiB) that wasm linear memory must also be a multiple of.  That's also true for asm.js memory sizes.  AsmJSPageSize was an internal constant that was asserted to be the same as the page size... but it didn't need to be, I could've just used the dynamically-determined page size (as done in this patch).  This is good b/c we're starting to see a variety of page sizes (16KiB).

> As this is larger than the requirements for both MIPS platforms, I assume
> this won't be an issue for the MIPS platforms?

Right.  The only interaction with OS page size is that, if we want to use mprotect/signals, we need the OS page size to be smaller and divisor of wasm::PageSize which should almost always be the case.

> > +#ifdef ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB
> > +static const uint64_t Uint32Range = uint64_t(UINT32_MAX) + 1;
> > +static const uint64_t MappedSize = 2 * Uint32Range + PageSize;
> 
> In the previous version, it was Uint32Range + constants, now 2 * Uint32Range
> + constants. Is that intended?

Yes: AsmJSImmediateRange was effectively Uint32Range so 2 * Uint32Range is just nice way to describe the range without pulling in JIT headers.  (Statically asserted, of course.)

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714360 [details] [diff] [review]
wasm-buffer

Review of attachment 8714360 [details] [diff] [review]:
-----------------------------------------------------------------

Much cleaner, thank you! Sorry the review took a bit long, but the initial code was very hard to read in splinter, and comparing before/after impls wasn't obvious. Now it's much better.

::: js/src/vm/ArrayBufferObject.cpp
@@ +374,5 @@
> +        return nullptr;
> +
> +    if (!VirtualAlloc(data, numBytes, MEM_COMMIT, PAGE_READWRITE)) {
> +        VirtualFree(data, 0, MEM_RELEASE);
> +        MemProfiler::RemoveNative(data);

pre-existing: Not too sure about this, but if you use RemoveNative, shouldn't you have used SampleNative in the first place? If so, maybe you can move the SampleNative call after this branch, so that you don't need to Sample/Remove in the case the second VirtualAlloc call failed.

@@ +398,5 @@
> +        return nullptr;
> +
> +    if (mprotect(data, numBytes, PROT_READ | PROT_WRITE)) {
> +        munmap(data, wasm::MappedSize);
> +        MemProfiler::RemoveNative(data);

ditto

@@ +437,5 @@
> +            return nullptr;
> +        }
> +
> +        BufferContents contents = BufferContents::create<WASM_MAPPED>(data);
> +        ArrayBufferObject* buffer = ArrayBufferObject::create(cx, numBytes, contents);

You could use auto* here too, if you wanted.

@@ +449,5 @@
> +        MOZ_CRASH("shouldn't be using signal handlers for out-of-bounds");
> +#endif
> +    }
> +
> +    auto buffer = ArrayBufferObject::create(cx, numBytes);

nit: auto*, to explicitly indicate pointer?

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714359 [details] [diff] [review]
move-constants

Review of attachment 8714359 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the explanation!

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714836 [details] [diff] [review]
fix-export-name

Review of attachment 8714836 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714837 [details] [diff] [review]
memory-section

Review of attachment 8714837 [details] [diff] [review]:
-----------------------------------------------------------------

Cool.

::: js/src/asmjs/WasmText.cpp
@@ +253,5 @@
>  };
>  
> +class WasmAstMemory : public WasmAstNode
> +{
> +    uint32_t initial_;

If you haven't read Wasm.cpp before this file, you might wonder what this "initial_" is. Could we name it initialSize_ or just size_, please?

::: js/src/jit-test/tests/wasm/basic.js
@@ +134,5 @@
> +assertErrorMessage(() => wasmEvalText('(module (memory 0))'), Error, /not a multiple of 0x10000/);
> +assertErrorMessage(() => wasmEvalText('(module (memory 1))'), Error, /not a multiple of 0x10000/);
> +assertErrorMessage(() => wasmEvalText('(module (memory 65535))'), Error, /not a multiple of 0x10000/);
> +assertErrorMessage(() => wasmEvalText('(module (memory 131071))'), Error, /not a multiple of 0x10000/);
> +assertErrorMessage(() => wasmEvalText('(module (memory 2147483648))'), Error, /initial memory size too big/);

Can you check with the biggest size which is less than INT32_MAX and a multiple of 0x10000, please?

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714838 [details] [diff] [review]
memory-export

Review of attachment 8714838 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I like the refactorings as well. A few more test cases and this is good to land.

::: js/src/asmjs/Wasm.cpp
@@ +547,5 @@
>      return mg.declareExport(Move(fieldName), funcIndex);
>  }
>  
>  static bool
> +DecodeMemoryExport(JSContext* cx, Decoder& d, ModuleGenerator& mg, CStringSet* dupSet)

dupSet is unused, and you probably don't want to allow several exports with the same name here too (probably the name check can be made common with DecodeFunctionExport). Can you add tests where there are memory exports of the same name, and one when two exports (1 function, 1 memory) have the same name too?

::: js/src/asmjs/WasmGenerator.cpp
@@ +372,5 @@
>          return exportMap_->fieldsToExports.append(p->value());
>      }
>  
>      uint32_t newExportIndex = module_->exports.length();
> +    if (newExportIndex == ExportMap::MemoryExport)

You could probably instead MOZ_ASSERT, because there's already a limit on the number of .exports (MaxExports in WasmBinary.h)

::: js/src/asmjs/WasmModule.cpp
@@ +53,5 @@
>  using mozilla::PodZero;
>  using mozilla::Swap;
>  using JS::GenericNaN;
>  
> +const uint32_t ExportMap::MemoryExport;

Is this declaration necessary?

@@ +1115,5 @@
>          if (!*fieldName) {
>              MOZ_ASSERT(!exportObj);
>              uint32_t exportIndex = exportMap.fieldsToExports[fieldIndex];
> +            if (exportIndex == ExportMap::MemoryExport) {
> +                exportObj.set(heap);

Maybe MOZ_ASSERT(heap); ?

::: js/src/asmjs/WasmText.cpp
@@ +1361,5 @@
>      return true;
>  }
>  
>  static bool
> +EncodeCString(Encoder& e, TwoByteChars twoByteChars)

Nice!

@@ +1482,5 @@
> +            if (!EncodeFunctionExport(e, *exp))
> +                return false;
> +            break;
> +          case WasmAstExportKind::Memory:
> +            if (!EncodeMemoryExport(e, *exp)) 

nit: trailing whitespace

::: js/src/jit-test/tests/wasm/basic.js
@@ +151,5 @@
> +
> +var obj = wasmEvalText('(module (memory 65536) (func (result i32) (i32.const 42)) (export "" memory) (export "a" 0) (export "b" 0))');
> +assertEq(obj instanceof ArrayBuffer, true);
> +assertEq(obj.a instanceof Function, true);
> +assertEq(obj.b instanceof Function, true);

Could we have a test in which memory is exported twice, and we assertEq the two exports are the same?

Comment 23

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #22)
Good points!

> > +const uint32_t ExportMap::MemoryExport;
> 
> Is this declaration necessary?

Yeah, it's C++ minutia: the in-class 'static const uint32_t MemoryExport = UINT32_MAX' is a *declaration* and *initialization* but not a *definition*.  Most of the time this doesn't matter b/c the initializer gets inlined and no definition is required for linking.  But if you use the static member variable in a way that can't be inlined (anything that ends up wanting a pointer; but in GCC often random stuff that you'd think would be inlined) it requires a definition which is necessarily out-of-line.  Blech.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42bf14535d13
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb583e242e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5711cc344772
https://hg.mozilla.org/integration/mozilla-inbound/rev/78998396c985
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc0fead84b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a534e3ded05

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42bf14535d13
https://hg.mozilla.org/mozilla-central/rev/aeb583e242e9
https://hg.mozilla.org/mozilla-central/rev/5711cc344772
https://hg.mozilla.org/mozilla-central/rev/78998396c985
https://hg.mozilla.org/mozilla-central/rev/0bc0fead84b9
https://hg.mozilla.org/mozilla-central/rev/0a534e3ded05