Closed Bug 1284156 Opened 4 years ago Closed 4 years ago

Baldr: add Memory/Table resizing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(9 files, 1 obsolete file)

No description provided.
Depends on: 1287967
Depends on: 1240984
Attached patch simpler-createSplinter Review
In the next patch, it is necessary to first create an ArrayBuffer and then later intialize it with contents.  Inbetween, the AB needs to be safely traceable/finalizable, but that's it.  This small patch adds a way to do that and uses it to slightly simplify one existing case.
Attachment #8789196 - Flags: review?(sphink)
This patch ensures that buffers taken from WebAssembly.Memory can't be stolen or detached.  It also removes some leftover stuff added by bug 1081379 but not removed when ArrayBuffer.transfer was removed.
Attachment #8789197 - Flags: review?(sphink)
Depends on: 1298202
Attached patch memory-grow-jsapi (obsolete) — Splinter Review
This patch adds 'grow' and implements the "always detach" semantics described in https://github.com/WebAssembly/design/pull/795.  If that gets merged then basically this completes memory growth.
Attachment #8789226 - Flags: review?(sunfish)
Comment on attachment 8789226 [details] [diff] [review]
memory-grow-jsapi

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

Great to see this coming! Can you try enabling the resizing.wast and memory_trap.wast tests, please?
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
Indeed they are already (as of a few hours ago) enabled on trunk by bug 1298202 :)
Attachment #8789196 - Flags: review?(sphink) → review+
Comment on attachment 8789197 [details] [diff] [review]
no-stealing-wasm-buffers

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1521,5 @@
>  
>      Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
>  
>      if (buffer->isWasm()) {
> +        JS_ReportError(cx, "Cannot detach WebAssembly ArrayBuffer");

Can you file a followup bug to create a js.msg entry for this error? Then it could be linked to a helpful MDN page that would explain.

::: js/src/vm/StructuredClone.cpp
@@ +1473,5 @@
>              JSAutoCompartment ac(context(), arrayBuffer);
>              size_t nbytes = arrayBuffer->byteLength();
>  
> +            if (arrayBuffer->isWasm()) {
> +                JS_ReportError(context(), "Cannot steal WebAssembly ArrayBuffer");

Ok. For other things, we've allowed transferring non-stealable ArrayBuffer by copying. But you're the expert on what WebAssembly wants, so I'm fine with this behavior.

@@ -1473,5 @@
>              JSAutoCompartment ac(context(), arrayBuffer);
>              size_t nbytes = arrayBuffer->byteLength();
>  
> -            // Structured cloning currently only has optimizations for mapped
> -            // and malloc'd buffers, not asm.js-ified buffers.

That comment sucked anyway. "has optimizations"?

@@ +1481,2 @@
>              bool hasStealableContents = arrayBuffer->hasStealableContents() &&
> +                                        (scope != JS::StructuredCloneScope::DifferentProcess);

I really ought to replace this mystery boolean with an enum { FORCE_COPY, ALLOW_TRANSFER }. Or something. Having hasStealableContents !== buffer->hasStealableContents() just sucks.

At least it's simpler after this patch.
Attachment #8789197 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #6)
Thanks for the quick review!

> > +            if (arrayBuffer->isWasm()) {
> > +                JS_ReportError(context(), "Cannot steal WebAssembly ArrayBuffer");
> 
> Ok. For other things, we've allowed transferring non-stealable ArrayBuffer
> by copying. But you're the expert on what WebAssembly wants, so I'm fine
> with this behavior.

I think in those other cases we sorta had to because the non-transferability was due to an impl detail / optimization.  Here, we have an opportunity to say: "no, you can't do that, if you want a copy, say so and know that you are paying for a copy".  But this is also a conservative stance: maybe people will push back and we could relax it later.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/741b0f3a2997
Add a way to safely initialize an ArrayBuffer after GC allocation (r=sfink)
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f2899d469f
Throw when attempting to steal/transfer an ArrayBuffer aliasing WebAssembly.Memory (r=sfink)
Whiteboard: [leave open]
Comment on attachment 8789226 [details] [diff] [review]
memory-grow-jsapi

Dan's working hard on 0xc so flipping over to Benjamin who was taking an interest anyway :)
Attachment #8789226 - Flags: review?(sunfish) → review?(bbouvier)
Attached patch segregate-asmjsSplinter Review
Oops, I realized that with wasm unconditionally detaching buffers on every grow, we need to disable linking asm.js to a wasm buffer.  There's also this corner case where we use a wasm buffer for SIMD.js.  This patch makes the flag "is prepared for asm.js" orthogonal from the BufferKind so we can cleanly express all four cases of (PLAIN, WASM) x (FOR_ASMJS, !FOR_ASMJS).  For reference:
 - array buffer before linking: * !FOR_ASMJS
 - buffer after linking normal asm.js: PLAIN FOR_ASMJS
 - buffer created by wasm: WASM !FOR_ASMJS
 - buffer after linking asm.js+SIMD.js: WASM FOR_ASMJS
Attachment #8789226 - Attachment is obsolete: true
Attachment #8789226 - Flags: review?(bbouvier)
Attachment #8789660 - Flags: review?(bbouvier)
Rebased on previous patch.
Attachment #8789662 - Flags: review?(bbouvier)
Comment on attachment 8789660 [details] [diff] [review]
segregate-asmjs

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

::: js/src/jit-test/tests/asm.js/testAsmJSWasmMixing.js
@@ +18,5 @@
> +    assertAsmLinkFail(simdJS, this, null, asmJSBuf);
> +    assertAsmLinkFail(simdJS, this, null, wasmMem.buffer);
> +
> +    var simdJSBuf = new ArrayBuffer(BUF_MIN);
> +    asmLink(simdJS, this, null, simdJSBuf);

Can you copy/paste this line once below, maybe with a comment, please? (since a wasm/asm.js buffer can be reused for a SIMD module)

::: js/src/vm/ArrayBufferObject.cpp
@@ +717,5 @@
>  
> +// Note this function can return false with or without an exception pending. The
> +// asm.js caller checks cx->isExceptionPending before propagating failure.
> +// Returning false without throwing means that asm.js linking will fail which
> +// will recompile as non-asm.js.

Should this comment be in the header file instead? Can you add MOZ_MUST_USE to the signature please?

@@ +1528,1 @@
>          JS_ReportError(cx, "Cannot detach WebAssembly ArrayBuffer");

Maybe update the error message too?

::: js/src/vm/ArrayBufferObject.h
@@ +193,5 @@
>  
>          // Views of this buffer might include typed objects.
> +        TYPED_OBJECT_VIEWS  = 0x20,
> +
> +        // TODO

nit: TODO
Attachment #8789660 - Flags: review?(bbouvier) → review+
Comment on attachment 8789662 [details] [diff] [review]
memory-grow-jsapi

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

Nice.

I see we detach the older array buffer in all cases (either by calling ABO::detach or ABO::stealContents), but the spec says it should only happen when no max is specified; is it the spec being out of date here?
https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymemoryprototypegrow

::: js/src/asmjs/WasmInstance.cpp
@@ +299,5 @@
>  
>  /* static */ uint32_t
>  Instance::currentMemory_i32(Instance* instance)
>  {
> +    uint32_t byteLength = instance->memory_->buffer().byteLength();

This could be instance->memoryLength() on the RHS

::: js/src/asmjs/WasmJS.h
@@ +189,5 @@
>      ArrayBufferObjectMaybeShared& buffer() const;
>  
>      bool movingGrowable() const;
>      bool addMovingGrowObserver(JSContext* cx, WasmInstanceObject* instance);
> +    static uint32_t grow(HandleWasmMemoryObject memory, uint32_t delta, JSContext* cx);

I think JSContext* should be the first argument here.

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +184,5 @@
> +assertEq(buf.byteLength, 0);
> +buf = mem.buffer;
> +assertEq(buf.byteLength, 2 * WasmPage);
> +assertErrorMessage(() => mem.grow(1), Error, /failed to grow memory/);
> +assertEq(buf, mem.buffer);

Can you also add tests (unless they're present somewhere else, in a similar form) where we:

- instantiate a module A
- export its memory
- instantiate a module B that reuses the memory of A
- call grow() from JS, and check that A and B are aware of the change (by trying an access that would be OOB before grow() and not thereafter, for instance)
- call grow() from a FFI called from wasm, and check that A and B are aware of the change

::: js/src/vm/ArrayBufferObject.h
@@ +348,5 @@
>      mozilla::Maybe<uint32_t> wasmMaxSize() const;
> +    static MOZ_MUST_USE bool wasmGrowToSizeInPlace(uint32_t newSize,
> +                                                   Handle<ArrayBufferObject*> oldBuf,
> +                                                   MutableHandle<ArrayBufferObject*> newBuf,
> +                                                   JSContext* cx);

Here and below, JSContext* should be the first argument (by convention in the rest of the engine)
Attachment #8789662 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> > +// Note this function can return false with or without an exception pending. The
> > +// asm.js caller checks cx->isExceptionPending before propagating failure.
> > +// Returning false without throwing means that asm.js linking will fail which
> > +// will recompile as non-asm.js.
> 
> Should this comment be in the header file instead?

I thought it was more useful in the definition since otherwise the code looks "wrong" and people might be tempted to "fix" it by adding throws (as already happened).

> >          JS_ReportError(cx, "Cannot detach WebAssembly ArrayBuffer");
> 
> Maybe update the error message too?

Well, the asm.js part is a bit of a cheat (semantically incorrect given asm.js is Just JS); I feel like calling it "WebAssembly" is sortof the handwavy excuse.
(In reply to Luke Wagner [:luke] from comment #15)
> I thought it was more useful in the definition since otherwise the code
> looks "wrong" and people might be tempted to "fix" it by adding throws (as
> already happened).

Sounds fair, maybe refer to this comment in the header instead?

> Well, the asm.js part is a bit of a cheat (semantically incorrect given
> asm.js is Just JS); I feel like calling it "WebAssembly" is sortof the
> handwavy excuse.
If a user sees this, and they're only using asm.js, they might wonder if this is a bug, and maybe open one on Bugzilla ;) Since this can only happen with SIMD.js, which isn't enabled on trunk, we can keep it as is in the meanwhile.
(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> I see we detach the older array buffer in all cases (either by calling
> ABO::detach or ABO::stealContents), but the spec says it should only happen
> when no max is specified; is it the spec being out of date here?
> https://github.com/WebAssembly/design/blob/master/JS.
> md#webassemblymemoryprototypegrow

Yeah, I mentioned this in comment 3.  I'm hopeful that will be merged but, if not, the work here is strictly additive: if we need for old ABs to keep aliasing memory, then we'll just need to add a branch and a weak map so that the non-owning ABs can keep the owning AB alive.

> > +    static uint32_t grow(HandleWasmMemoryObject memory, uint32_t delta, JSContext* cx);
> 
> I think JSContext* should be the first argument here.

So this is the somewhat-lesser-known SM convention that: to signify that a cx-taking function doesn't throw (it only needs the cx for administrative purposes like rooting), you take 'cx' as the last argument.  And that indeed is the case in the 3 functions you've observed in this path.
(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
> Since this can only happen with SIMD.js, which isn't enabled on trunk, 
> we can keep it as is in the meanwhile.

Well, actually it's all asm.js.  In the past we reported "out of memory".  I guess you're right though, and it should say "asm.js".
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6bdd39e759b
Baldr: prevent use of wasm buffers for asm.js (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5114a02a7b19
Baldr: add Memory.grow, fix ArrayBuffer (r=bbouvier)
Summary: Baldr: add Memory/Table resizing (and grow_memory) → Baldr: add Memory/Table resizing
The current scheme for dealing with indirect calls to null saves a null check but adds a fair amount of complexity because Tables now need an Instance to create null elements.  Table.grow() would exacerbate this so to avoid all this this patch simply does the null-check instead.
Attachment #8789927 - Flags: review?(bbouvier)
Attached patch error-msgSplinter Review
This patch splits the vague "bad wasm indirect call" error into one for calls to null vs. calls with a signature mismatch.
Attachment #8789928 - Flags: review?(bbouvier)
Attached patch table-resizeSplinter Review
This patch adds Table.resize().
Attachment #8789930 - Flags: review?(bbouvier)
Comment on attachment 8789927 [details] [diff] [review]
rm-bad-indirect-call

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

Nice improvement.

::: js/src/asmjs/WasmInstance.cpp
@@ +854,5 @@
>          void** array = table->internalArray();
>          uint32_t length = table->length();
> +        for (size_t i = 0; i < length; i++) {
> +            if (array[i])
> +                UpdateEntry(*code_, newProfilingEnabled, &array[i]);

Not sure it is strictly needed at the moment, since this is only enabled for asm.js which maintains (and doesn't export) its own tables.

::: js/src/asmjs/WasmJS.cpp
@@ -1209,5 @@
>      Instance& instance = *elem.tls->instance;
> -    const CodeRange* codeRange = instance.code().lookupRange(elem.code);
> -
> -    // A non-function code range means the bad-indirect-call stub, so a null element.
> -    if (!codeRange || !codeRange->isFunction()) {

Can you make it a MOZ_ASSERT(codeRange.isFunction()); instead?
Attachment #8789927 - Flags: review?(bbouvier) → review+
Comment on attachment 8789928 [details] [diff] [review]
error-msg

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

Sweet
Attachment #8789928 - Flags: review?(bbouvier) → review+
Attached patch barriersSplinter Review
I realized that we need a write barrier in Table.prototype.set() since it can make WasmInstanceObject's unreachable in the middle of igc.
Attachment #8790418 - Flags: review?(sphink)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b192dfc55615
Baldr: simplify representation of null table elements (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f38a26f218
Baldr: split 'bad indirect call' error message (r=bbouvier)
Attachment #8790418 - Flags: review?(sphink) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d94933493a
Baldr: add missing Table.set post-barrier (r=sfink)
Following:
https://github.com/WebAssembly/design/blob/master/JS.md#tononwrappinguint32

This also fills in a missing Table constructor initial>maximum check.
Attachment #8791015 - Flags: review?(bbouvier)
Comment on attachment 8791015 [details] [diff] [review]
to-non-wrapping-uint32

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

Much cleaner!

::: js/src/asmjs/WasmJS.cpp
@@ +1227,5 @@
>      if (!args.requireAtLeast(cx, "set", 2))
>          return false;
>  
> +    uint32_t index;
> +    if (!ToNonWrappingUint32(cx, args.get(0), table.length() - 1, "Table", "get index", &index))

nit: set index

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +169,5 @@
>  assertEq(memGrow.length, 1);
>  assertErrorMessage(() => memGrow.call(), TypeError, /called on incompatible undefined/);
>  assertErrorMessage(() => memGrow.call({}), TypeError, /called on incompatible Object/);
> +assertErrorMessage(() => memGrow.call(mem1, -1), Error, /bad Memory grow delta/);
> +assertErrorMessage(() => memGrow.call(mem1, Math.pow(2,32)), Error, /bad Memory grow delta/);

Should these two check for a RangeError too?

@@ +281,5 @@
>  assertErrorMessage(() => set.call({}), TypeError, /called on incompatible Object/);
>  assertErrorMessage(() => set.call(tbl1, 0), TypeError, /requires more than 1 argument/);
> +assertErrorMessage(() => set.call(tbl1, 2, null), RangeError, /bad Table get index/);
> +assertErrorMessage(() => set.call(tbl1, -1, null), RangeError, /bad Table get index/);
> +assertErrorMessage(() => set.call(tbl1, Math.pow(2,33), null), RangeError, /bad Table get index/);

(nit x3: set)
Attachment #8791015 - Flags: review?(bbouvier) → review+
Comment on attachment 8789930 [details] [diff] [review]
table-resize

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

Great to have this, thanks for the patch.

::: js/src/asmjs/WasmGenerator.cpp
@@ +164,5 @@
>          for (uint32_t i = 0; i < numSigs_; i++) {
>              SigWithId& sig = shared_->sigs[i];
>              if (SigIdDesc::isGlobal(sig)) {
>                  uint32_t globalDataOffset;
> +                if (!allocateGlobalBytes(sizeof(TableTls), sizeof(void*), &globalDataOffset))

nit: i think this isn't needed.

::: js/src/asmjs/WasmInstance.h
@@ +64,5 @@
>      // Only WasmInstanceObject can call the private trace function.
>      friend class js::WasmInstanceObject;
>      void tracePrivate(JSTracer* trc);
>  
> +    // Only Wasm(Memory|Table)Object can call the private onMovingGrow notifications.

nit: mismatch with below friend declarations (with Table, not WasmTableObject)

::: js/src/asmjs/WasmModule.cpp
@@ +560,5 @@
> +        return false;
> +    }
> +
> +    if ((actualMax.isSome() && (!declaredMax.isSome() || actualMax.value() > declaredMax.value())) ||
> +        (actualMax.isNothing() && !declaredMax.isNothing()))

Personal taste, but I'd use the bool conversion operators here rather than the isSome()/isNothing(), to make it a bit more concise.

@@ +591,5 @@
>          MOZ_ASSERT_IF(!metadata_->isAsmJS(), buffer.as<ArrayBufferObject>().isWasm());
>  
> +        if (!CheckResizableLimits(cx, declaredMin, declaredMax,
> +                                  buffer.byteLength(), buffer.wasmMaxSize(),
> +                                  metadata_->isAsmJS(), "Memory")) {

nit: multi-line condition => { on next line

@@ +629,5 @@
>  
>          Table& table = tableObj->table();
> +        if (!CheckResizableLimits(cx, td.limits.initial, td.limits.maximum,
> +                                  table.length(), table.maximum(),
> +                                  metadata_->isAsmJS(), "Table")) {

(ditto here)

::: js/src/asmjs/WasmTable.cpp
@@ +143,5 @@
> +    if (maximum_ && newLength.value() > maximum_.value())
> +        return -1;
> +
> +    if (length_ == newLength.value())
> +        return length_;

This could be hoisted up and simply written as

if (!delta)

@@ +155,5 @@
> +        return -1;
> +
> +    PodZero(newArray + length_, delta);
> +
> +    Unused << array_.release();

Can you add a small comment that externalArray() == array_ we just realloced, thus this is safe to release?

@@ +170,5 @@
> +
> +bool
> +Table::movingGrowable() const
> +{
> +    return !maximum_ || length_ != maximum_.value();

Can you either change the != to a <, or MOZ_ASSERT(length_ <= maximum_.value()) ?

::: js/src/asmjs/WasmTypes.h
@@ +1049,5 @@
>      }
>      static CalleeDesc asmJSTable(const TableDesc& desc) {
>          CalleeDesc c;
>          c.which_ = AsmJSTable;
> +        c.u.table.globalDataOffset_ = desc.globalDataOffset;

Just to make it obvious, can you also set c.u.table.external_ = false?

@@ +1080,5 @@
>          return which_ == WasmTable || which_ == AsmJSTable;
>      }
>      uint32_t tableGlobalDataOffset() const {
>          MOZ_ASSERT(isTable());
> +        return u.table.globalDataOffset_;

In terms of API, wouldn't it be better that we'd have two functions

uint32_t tableNumElems() const
void* tableBase() const

that return u.table.globalDataOffset_ + offsetof(TableTls, numElems/base), instead? (it would require putting TableTls before this, of course)

Otherwise, having the offsetof(TableTls, base) leaking out (e.g. in MacroAssembler.cpp) looks like an impl. detail the caller shouldn't be aware of.

@@ +1172,5 @@
> +
> +struct TableTls
> +{
> +    // Length of the table in number of elements (not bytes).
> +    uint32_t length;

Light suggestion: rename to numElems, then?

::: js/src/jit-test/tests/wasm/resizing.js
@@ +145,5 @@
> +
> +setJitCompilerOption("baseline.warmup.trigger", 2);
> +setJitCompilerOption("ion.warmup.trigger", 4);
> +for (var i = 0; i < 10; i++)
> +    assertEq(exports.test(), 3);

Can you also assertEq(exports.tbl.length, 11) outside the loop?
Attachment #8789930 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #32)
Thanks, great points, applied.

> > +                if (!allocateGlobalBytes(sizeof(TableTls), sizeof(void*), &globalDataOffset))
> 
> nit: i think this isn't needed.

Good catch!

> Personal taste, but I'd use the bool conversion operators here rather than
> the isSome()/isNothing(), to make it a bit more concise.

Oh, yes, agreed!  (I keep mixing up Maybe with CheckedInt, which only has the explicit methods.)

> > +        if (!CheckResizableLimits(cx, declaredMin, declaredMax,
> > +                                  buffer.byteLength(), buffer.wasmMaxSize(),
> > +                                  metadata_->isAsmJS(), "Memory")) {
> 
> nit: multi-line condition => { on next line

In general, yes, but see the "However, if there is already a visual separation between the condition and the body, putting the { on a new line isn't necessary: " exception in https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#Other_whitespace :)

To wit: historically SM style *required* you to put the { on the same line as the conditional and then we pushed for you to be *allowed* to put the { on the new line to provide a visual break in the case where the condition was at the column as the body.  (Nowadays I just just try to avoid multi-line anything b/c it's ugly.)

> > +    if (length_ == newLength.value())
> > +        return length_;
> 
> This could be hoisted up and simply written as

I put this here to make the connection with the following assert (which depends on this) more clear, but probably I should do what you suggest and then comment the dependency.

> > +        c.u.table.globalDataOffset_ = desc.globalDataOffset;
> 
> Just to make it obvious, can you also set c.u.table.external_ = false?

Well, that field cannot even be accessed if which_ == AsmJSTable; if we split c.u.table into c.u.wasmTable/c.u.asmJSTable, it wouldn't exist in asmJSTable.

> Otherwise, having the offsetof(TableTls, base) leaking out (e.g. in
> MacroAssembler.cpp) looks like an impl. detail the caller shouldn't be aware
> of.

I wouldn't exactly consider TableTls an impl detail of CalleeDesc (since it's not encapsulated in any way), but I do like this change and think it makes the client code easier to read.

> > +    // Length of the table in number of elements (not bytes).
> > +    uint32_t length;
> 
> Light suggestion: rename to numElems, then?

I'd kinda like to keep using 'length' to match up with Table.prototype.length and the general style of 'length' meaning "number of elements" and 'size' meaning "number of bytes".
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a175eed324
Baldr: add Table.prototype.grow (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/be756757b11f
Baldr: use ToNonWrappingUint32 for range checks (r=bbouvier)
Luke, this work seems done unless you had something more in mind, can we close this one?
Flags: needinfo?(luke)
Indeed!
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(luke)
Resolution: --- → FIXED
Whiteboard: [leave open]
It seems that the first 2 patches regressed all the *.c tests from JetStream on the "Compare OS" mode from AWFY.
Duplicate of this bug: 1036786
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.