Closed Bug 1416179 Opened 2 years ago Closed 8 months ago

[MIPS64] Add wasm huge memory support

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox58 --- fix-optional

People

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

Details

Attachments

(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]
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.
Assignee: nobody → dragan.mladjenovic
Status: UNCONFIRMED → ASSIGNED
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]
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.
Attachment #8927932 - Flags: review?(lhansen) → review+
Pushed by shindli@mozilla.com:
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
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 ncsoregi@mozilla.com:
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
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)
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Flags: needinfo?(ajones)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.