Closed Bug 1416179 Opened 5 years ago Closed 4 years ago

[MIPS64] Add wasm huge memory support


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




Tracking Status
firefox58 --- fix-optional


(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)



(2 files, 1 obsolete file)

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 .
Attached patch bug1416179_maxSizeWorkaroud.diff (obsolete) — Splinter Review
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.
Attachment #8927268 - Flags: review?(lhansen)
Comment on attachment 8927268 [details] [diff] [review]

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.
Assignee: nobody → dragan.mladjenovic
Ever confirmed: true
Severity: normal → enhancement
Priority: -- → P5
Attachment #8927268 - Attachment is obsolete: true
Attachment #8927268 - Flags: review?(lhansen)
Attachment #8927932 - Flags: review?(lhansen)
Comment on attachment 8927932 [details] [diff] [review]

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.
Attachment #8927932 - Flags: review?(lhansen) → review+
Pushed by
Prevent ArrayBufferObject from reserving memory larger than UINT32_MAX on !WASM_HUGE_MEMORY 64-bit platforms; r=lth
Keywords: checkin-needed
Previous patch mishandled the case when user specifies 65535 as max number of memory pages. This one fixes it.
Attachment #8936359 - Flags: review?(lhansen)
Attachment #8936359 - Flags: review?(lhansen) → review+
Keywords: checkin-needed
Pushed by
Change ArrayBufferObject's !WASM_HUGE_MEMORY 64-bit max memory validation to handle values less than UINT32_MAX that lead to overflow; r=lth
Keywords: checkin-needed
Component: JavaScript Engine: JIT → Javascript: Web Assembly

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

Flags: needinfo?(ajones)
Closed: 4 years ago
Flags: needinfo?(ajones)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.