Closed Bug 1295034 Opened 3 years ago Closed 3 years ago

Self-hosted JavaScript assertion info: "builtin/Sorting.js:105: Attached data buffer should be reified when array length is >= 128."

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 6e191a55c3d2 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --no-threads --ion-eager):

const constructors = [
    Int8Array,
    Uint8Array,
    Uint8ClampedArray,
    Int16Array,
    Uint16Array,
    Uint32Array,
    Float32Array,
    Float64Array ];
for (var ctor of constructors) {
    var testArray = new Int32Array(1024);
    testArray.sort();
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x08754b6a in intrinsic_AssertionFailed (cx=0xf7948000, argc=1, vp=0xffffc0d8) at js/src/vm/SelfHosting.cpp:401
#0  0x08754b6a in intrinsic_AssertionFailed (cx=0xf7948000, argc=1, vp=0xffffc0d8) at js/src/vm/SelfHosting.cpp:401
#1  0x086f831b in js::CallJSNative (cx=0xf7948000, native=0x8754ae0 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#2  0x086ef5a6 in js::InternalCallOrConstruct (cx=0xf7948000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453
#3  0x086ef8dd in InternalCall (cx=cx@entry=0xf7948000, args=...) at js/src/vm/Interpreter.cpp:498
#4  0x086efa2f in js::CallFromStack (cx=0xf7948000, args=...) at js/src/vm/Interpreter.cpp:504
#5  0x089ce658 in js::jit::DoCallFallback (cx=0xf7948000, frame=0xffffc138, stub_=0xf79c4200, argc=1, vp=0xffffc0d8, res=...) at js/src/jit/BaselineIC.cpp:5981
#6  0xf7fc467c in ?? ()
[...]
#10 0xf7b79a20 in ?? ()
eax	0x0	0
ebx	0x8c23ff4	146948084
ecx	0xf7d9c864	-136722332
edx	0x0	0
esi	0xf7d9bdf8	-136725000
edi	0x1	1
ebp	0xffffbd98	4294950296
esp	0xffffbd80	4294950272
eip	0x8754b6a <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+138>
=> 0x8754b6a <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+138>:	movl   $0x0,0x0
   0x8754b74 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+148>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160811131522" and the hash "7d5bcb47cee76da9491158d4f69b3b449ec5fea2".
The "bad" changeset has the timestamp "20160811134122" and the hash "f3c3c5beafc7de2957be8b4b3322e565d52cd79b".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7d5bcb47cee76da9491158d4f69b3b449ec5fea2&tochange=f3c3c5beafc7de2957be8b4b3322e565d52cd79b
Inlined TypedArray constructors (bug 1276955) don't call maybeCreateArrayBuffer [1], if I understand this comment correctly (bug 1276955, comment 16). That means we can get a |null| buffer from ViewedArrayBufferIfReified [2] even if the array length exceeds 127. 


[1] http://hg.mozilla.org/mozilla-central/file/6e191a55c3d2/js/src/vm/TypedArrayObject.cpp#l880
[2] http://hg.mozilla.org/mozilla-central/file/6e191a55c3d2/js/src/builtin/TypedArray.js#l7
Andre: I am at work right now so I can't fully look into it, but do the build options have something to do with it? The test that I wrote had the exact same case and it didn't fail. Neither is the attached test crashing on my build.
Adding Morgan to cc list, who was the reviewer for bug 1290566, the possible regressor.
Blocks: 1290566
(In reply to Sumit Tiwari from comment #3)
> Andre: I am at work right now so I can't fully look into it, but do the
> build options have something to do with it? The test that I wrote had the
> exact same case and it didn't fail. Neither is the attached test crashing on
> my build.

You can use the default debug-build options (--enable-debug) to reproduce the assertion error. It's only important to run the SpiderMonkey shell with --ion-eager --no-threads (or --ion-eager --ion-offthread-compile=off). Alternatively you can increase the loop count and use the default shell options, like:

for (var i = 0; i < 100000; ++i) {
    var testArray = new Int32Array(1024);
    testArray.sort();
}
(In reply to Sumit Tiwari from comment #3)
> Andre: I am at work right now so I can't fully look into it, but do the
> build options have something to do with it? The test that I wrote had the
> exact same case and it didn't fail. Neither is the attached test crashing on
> my build.

This happens often enough. The trouble is, TypedArray's will behave differently after moving from the interpreter to the jits. That's why it shows up after many iterations, or with special flags set, but not otherwise. My bad for not suggesting a test case that would have revealed it.
Morgan, Andre: Thanks for the help. I finally had some time to look into it and you were right - TypedArray behaves differently under JIT (as Andre points out, it's described in bug 1276995).

I believe my solution for bug 1290566 is still correct - if the buffer property is explicitly defined, then the TypedArray will ALWAYS have a buffer attached, regardless of whether or not it's under the JIT compiler (I verified this). So all we need to do is find a better way to check for this buffer, or remove it entirely (atleast from the place where it's currently at). 

Am I correct or am I still missing something? I apologize in advance - still a beginner to SpiderMonkey and its internals.
I guess the easiest way to fix this issue is to ensure the ArrayBuffer gets reified by calling the TypedArray.prototype.buffer function [1]. You can access this getter function by exporting it in vm/SelfHosting.cpp as "std_TypedArray_buffer" [2], similar to how Object functions are being made available for the self-hosting global [3,4]. And in builtin/Sorting.js we can then call the getter when the buffer is null: |if (buffer === null) buffer = callFunction(std_TypedArray_buffer, array);|.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.cpp#1327
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/SelfHosting.cpp#2339
[3] https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Object.h#38
[4] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/SelfHosting.cpp#2304
That certainly seems like a viable solution.

I am more curious about why we don't attach a buffer when we create TypedArrays under JIT. It seems that with the solution above, all TypedArrays will always have a buffer attached (for now, this would occur only if they are being sorted, but other cases that require a buffer might pop up). Wouldn't it then be easier to simply create a buffer when constructing the objects in the first place, thus removing the need to explicitly check for a buffer? Unless, of course, there is a technical limitation that prohibits us from doing so. 

Just thinking out loud; I am totally up for implementing Andre's quick fix.
(In reply to Sumit Tiwari from comment #9)
> I am more curious about why we don't attach a buffer when we create
> TypedArrays under JIT.

That's just done for performance reasons, by storing the typed array's data inline we can avoid creating a separate (ArrayBuffer) object.
I was able to implement Andre's suggested fix to get around this problem. Will push out a review soon.
(In reply to Sumit Tiwari from comment #11)
> I was able to implement Andre's suggested fix to get around this problem.
> Will push out a review soon.

Thanks Sumit!
I sent out a review implementing Andre's suggested fix (with some help from arai). I can't comment on the performance impact, if any, as my machine is simply too slow, but atleast no regressions from what I could test.
Comment on attachment 8783779 [details]
Bug 1295034 - Assertion failure when sorting TypedArrays constructed under JIT;

https://reviewboard.mozilla.org/r/73458/#review71466

::: js/src/builtin/Sorting.js:104
(Diff revision 1)
>      if (len < 128) {
>          QuickSort(array, len, comparefn);
>          return array;
>      }
>  
> +    // This happens if the array object is constructed under JIT

We only need to reify the buffer for the floating point case, that means the if-statement and assertion should be moved into the "if (floating) { ... }" block.

::: js/src/vm/SelfHosting.cpp:2286
(Diff revision 1)
>      JS_INLINABLE_FN("std_Array_slice",           array_slice,                  2,0, ArraySlice),
>      JS_FN("std_Array_sort",                      array_sort,                   1,0),
>      JS_FN("std_Array_reverse",                   array_reverse,                0,0),
>      JS_INLINABLE_FN("std_Array_splice",          array_splice,                 2,0, ArraySplice),
> +    
> +    JS_FN("std_TypedArray_buffer",               js::TypedArray_bufferGetter,  1,0),

std_TypedArray_buffer should be moved after std_String_concat to keep the entries in alphabetical order.

::: js/src/vm/TypedArrayObject.h:297
(Diff revision 1)
>      static bool is(HandleValue v);
>  
>      static bool set(JSContext* cx, unsigned argc, Value* vp);
>  };
>  
> +bool TypedArray_bufferGetter(JSContext* cx, unsigned argc, Value* vp);

Please add MOZ_MUST_USE to this declaration.

::: js/src/vm/TypedArrayObject.cpp:1376
(Diff revision 1)
>      JS_SELF_HOSTED_FN("entries", "TypedArrayEntries", 0, 0),
>      JS_SELF_HOSTED_FN("keys", "TypedArrayKeys", 0, 0),
>      JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_FN("includes", "TypedArrayIncludes", 2, 0),
> +    JS_FN("bufferGetter", js::TypedArray_bufferGetter, 1, 0),

Please remove this line, we don't need to add "bufferGetter" as a new property on %TypedArray%.prototype.
Comment on attachment 8783779 [details]
Bug 1295034 - Assertion failure when sorting TypedArrays constructed under JIT;

https://reviewboard.mozilla.org/r/73458/#review71530

Andre covered everything in his review, I'll also add that we need a test in this patch. Something like: 

const constructors = [
  Int8Array,
  Uint8Array,
  Uint8ClampedArray,
  Int16Array,
  Uint16Array,
  Uint32Array,
  Float32Array,
  Float64Array ]

// Ensure that sorting behaves properly when jitted.
for (var _ of Array(1024)) {
  for (var ctor of constructors) {
    var testArray = new Int32Array(1024)
    testArray.sort()
  }
}

It's almost there. Thanks for doing this! :) Look forward to seeing the next version.

::: js/src/vm/SelfHosting.cpp:2286
(Diff revision 1)
>      JS_INLINABLE_FN("std_Array_slice",           array_slice,                  2,0, ArraySlice),
>      JS_FN("std_Array_sort",                      array_sort,                   1,0),
>      JS_FN("std_Array_reverse",                   array_reverse,                0,0),
>      JS_INLINABLE_FN("std_Array_splice",          array_splice,                 2,0, ArraySplice),
> +    
> +    JS_FN("std_TypedArray_buffer",               js::TypedArray_bufferGetter,  1,0),

We should change the name of this from std_TypedArray_buffer to _bufferGetter or something along those lines IMO.

::: js/src/vm/TypedArrayObject.cpp:1376
(Diff revision 1)
>      JS_SELF_HOSTED_FN("entries", "TypedArrayEntries", 0, 0),
>      JS_SELF_HOSTED_FN("keys", "TypedArrayKeys", 0, 0),
>      JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_FN("includes", "TypedArrayIncludes", 2, 0),
> +    JS_FN("bufferGetter", js::TypedArray_bufferGetter, 1, 0),

We definitely don't want to expose this.
Attachment #8783779 - Flags: review?(winter2718)
Thanks for the feedback, Andre and Morgan; sent out a review addressing it. Please only see the diff between v0 and v2, and not v1 and v2, as I had mistakenly modified an older version of Sorting.js.
Comment on attachment 8783779 [details]
Bug 1295034 - Assertion failure when sorting TypedArrays constructed under JIT;

https://reviewboard.mozilla.org/r/73458/#review71644

Let's make sure this passes on try runs and ship it. One request, could you throw up a patch with the bug number in the test comments? If not, I don't mind adding the comment before landing. Thanks for doing this!

::: js/src/tests/ecma_6/TypedArray/sort_jit_array.js:13
(Diff revision 2)
> +    Uint32Array,
> +    Float32Array,
> +    Float64Array ];
> +
> +// Ensure that when creating Typed arrays under JIT
> +// the sort() method works as expected.

Nit: I forgot to mention this previously, it would be nice to have a bug number here.
Attachment #8783779 - Flags: review?(winter2718) → review+
Added a reference to the bug in the test. Thanks for the feedback, Morgan!
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/024e7dc7a219
Assertion failure when sorting TypedArrays constructed under JIT; r=mrrrgn
AFAIK, the test times out because it just takes too long. On my relatively-fast machine, it takes about 30 seconds to finish that particular test. That might have to do something with the loop size, but I am not too sure about this.
Here's a replacement for the test case, this one should complete faster. If it's still too slow we should remove |Uint8ClampedArray| from the |constructors| array, because uint8-clamped arrays seem to require more time when calling sort(). Please note that this test should go under js/src/jit-tests instead of js/src/tests.

---
setJitCompilerOption("ion.warmup.trigger", 40);

const constructors = [
    Int8Array,
    Uint8Array,
    Uint8ClampedArray,
    Int16Array,
    Uint16Array,
    Int32Array,
    Uint32Array,
    Float32Array,
    Float64Array ];

// Ensure that when creating TypedArrays under JIT
// the sort() method works as expected (bug 1295034).
for (var ctor of constructors) {
    do {
        var testArray = new ctor(1024);
        testArray.sort();
    } while (!inIon());
}
---
Oy, a push with André's suggested changes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a981a957a3f
Flags: needinfo?(winter2718)
Thank you for the suggestion Andre, and thanks Morgan for pushing the revision with Andre's suggested changes (you beat me by just a minute). :)

I apologize for not having thought of this sooner - this is my first time dealing with an IonMonkey specific issue.
(In reply to Sumit Tiwari from comment #28)
> Thank you for the suggestion Andre, and thanks Morgan for pushing the
> revision with Andre's suggested changes (you beat me by just a minute). :)
> 
> I apologize for not having thought of this sooner - this is my first time
> dealing with an IonMonkey specific issue.

I'll land the modified patch. You've been awesome Sumit, thank you x100. :) Hope you'll take some more bugs soon! ;]
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb039b3ce404
Assertion failure when sorting TypedArrays constructed under JIT; r=mrrrgn
(In reply to Morgan Phillips [:mrrrgn] from comment #29)
> (In reply to Sumit Tiwari from comment #28)
> > Thank you for the suggestion Andre, and thanks Morgan for pushing the
> > revision with Andre's suggested changes (you beat me by just a minute). :)
> > 
> > I apologize for not having thought of this sooner - this is my first time
> > dealing with an IonMonkey specific issue.
> 
> I'll land the modified patch. You've been awesome Sumit, thank you x100. :)
> Hope you'll take some more bugs soon! ;]

Thank you for the kind words, Morgan; and thanks again for landing the patch.
https://hg.mozilla.org/mozilla-central/rev/cb039b3ce404
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.