1416179 - [MIPS64] Add wasm huge memory support

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P5)

Description

[Dragan Mladjenovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36



Actual results:

The MIPS64 Wasm support does not define WASM_HUGE_MEMORY .

Comment 1

[Dragan Mladjenovic]

Attachment: bug1416179_maxSizeWorkaroud.diff

Temporary workaround that prevents reserving more that UINT32_MAX size memory on mips64. This allows us to use 32-bit wasm bounds checking support for the time being.

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8927268 [details] [diff] [review]
bug1416179_maxSizeWorkaroud.diff

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +717,5 @@
>          uint32_t clamp = Max(OneGiB, initialSize);
>          maxSize = Some(Min(clamp, maybeMaxSize.value()));
>      }
>  
> +#ifdef JS_CODEGEN_MIPS64

I think what we want here is #ifndef WASM_HUGE_MEMORY, and then an additional guard in the 'if' on sizeof(void*) == 8.  The reason is that other 64-bit platforms might also not have WASM_HUGE_MEMORY; for at least the initial work on ARM64 I expect this to be the case for example.

@@ +719,5 @@
>      }
>  
> +#ifdef JS_CODEGEN_MIPS64
> +    if (maybeMaxSize && maybeMaxSize == UINT32_MAX) {
> +        // MIPS64 doesn't define WASM_HUGE_MEMORY, se bug 1416179

This line referencing MIPS64 can then be removed.

@@ +725,5 @@
> +        // maxSize + wasm::PageSize < UINT32_MAX and maxSize % wasm::PageSize == 0
> +        maxSize = (wasm::MaxMemoryMaximumPages-2)*wasm::PageSize;
> +        MOZ_ASSERT(maxSize < UINT32_MAX);
> +        MOZ_ASSERT(initialSize <= maxSize);
> +    }

This code is probably OK, but before I r+ this patch I need to have a discussion with Luke about how we handle allocations > 2GB on 32-bit systems.

Comment 3

[Dragan Mladjenovic]

Attachment: bug1416179_maxSizeWorkaroud2.diff

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8927932 [details] [diff] [review]
bug1416179_maxSizeWorkaroud2.diff

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

OK.  I think neither Luke nor I are completely sure whether this has to subtract PageSize*2 or just PageSize, but it's fine the way it is either way, as 2GB is the hard limit on an ArrayBuffer allocation and that is (or should be) checked along all existing paths.  I'm landing some code soon that hardens that checking even further.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7141bc93ea02
Prevent ArrayBufferObject from reserving memory larger than UINT32_MAX on !WASM_HUGE_MEMORY 64-bit platforms; r=lth

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7141bc93ea02

Comment 7

[Dragan Mladjenovic]

Attachment: bug1416179_workaround_fix.patch

Previous patch mishandled the case when user specifies 65535 as max number of memory pages. This one fixes it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad3be99f87e
Change ArrayBufferObject's !WASM_HUGE_MEMORY 64-bit max memory validation to handle values less than UINT32_MAX that lead to overflow; r=lth

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fad3be99f87e

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ajones, maybe it's time to close this bug?