Closed
Bug 1312488
Opened 8 years ago
Closed 8 years ago
Assertion failure: isDouble(), at dist/include/js/Value.h:331
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | fixed |
People
(Reporter: decoder, Assigned: Waldo)
References
Details
(4 keywords, Whiteboard: [fuzzblocker] [jsbugmon:])
Attachments
(3 files)
1.94 KB,
patch
|
Details | Diff | Splinter Review | |
820 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
721 bytes,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 215f96861176 (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 --fuzzing-safe --ion-eager --ion-offthread-compile=off): [0, 4.4].sort(Array.some) Backtrace: received signal SIGSEGV, Segmentation fault. 0x087562ca in JS::Value::setDouble (d=<optimized out>, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:331 #0 0x087562ca in JS::Value::setDouble (d=<optimized out>, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:331 #1 js::ObjectElements::ConvertElementsToDoubles (cx=0xf7953000, elementsPtr=4048978064) at js/src/vm/NativeObject.cpp:77 #2 0xf7be845d in ?? () Backtrace stopped: previous frame inner to this frame (corrupt stack?) eax 0x0 0 ebx 0x8c70ff4 147263476 ecx 0xf7da4864 -136689564 edx 0x0 0 esi 0x0 0 edi 0x2 2 ebp 0xffffb198 4294947224 esp 0xffffb190 4294947216 eip 0x87562ca <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+106> => 0x87562ca <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+106>: movl $0x0,0x0 0x87562d4 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+116>: ud2 This happens with high frequency, marking as fuzzblocker.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:bisect]
Comment 1•8 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Updated•8 years ago
|
Whiteboard: [fuzzblocker] [jsbugmon:bisect] → [fuzzblocker] [jsbugmon:]
Comment 2•8 years ago
|
||
Fallout from bug 1243617 maybe?
Reporter | ||
Comment 3•8 years ago
|
||
We need a fix for this quickly, this issue is breaking most of the fuzzing because it's triggering so often. Jan, can you help us here? Thanks!
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 4•8 years ago
|
||
I'll look now (or at least quickly hand back), since jandem's probably not going to be around for several hours.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 5•8 years ago
|
||
I can't reproduce with my compiler, gcc 6.2.1, Red Hat flavor. With mine, I compile js::ObjectElements::ConvertElementsToDoubles's code just prior to/after the loop as follows: (gdb) disas Dump of assembler code for function js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int): ... 0x0872ac4b <+59>: xor %eax,%eax 0x0872ac4d <+61>: jmp 0x872ac57 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+71> 0x0872ac4f <+63>: nop 0x0872ac50 <+64>: add $0x1,%eax 0x0872ac53 <+67>: cmp %eax,%ecx 0x0872ac55 <+69>: je 0x872ac90 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+128> => 0x0872ac57 <+71>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x0872ac5c <+76>: jne 0x872ac50 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+64> 0x0872ac5e <+78>: fildl (%edx,%eax,8) 0x0872ac61 <+81>: fstpl (%edx,%eax,8) 0x0872ac64 <+84>: cmpl $0xffffff80,0x4(%edx,%eax,8) 0x0872ac69 <+89>: jbe 0x872ac50 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+64> 0x0872ac6b <+91>: lea -0x1e9f58(%ebx),%edx 0x0872ac71 <+97>: lea -0x21ee6e(%ebx),%eax 0x0872ac77 <+103>: mov $0x14b,%ecx 0x0872ac7c <+108>: call 0x8073ce6 <MOZ_ReportAssertionFailure(char const*, char const*, int)> 0x0872ac81 <+113>: movl $0x0,0x0 0x0872ac8b <+123>: ud2 0x0872ac8d <+125>: lea 0x0(%esi),%esi 0x0872ac90 <+128>: mov -0x10(%edx),%eax 0x0872ac93 <+131>: or $0x1,%eax 0x0872ac96 <+134>: mov %eax,-0x10(%edx) 0x0872ac99 <+137>: add $0x4,%esp 0x0872ac9c <+140>: mov $0x1,%eax 0x0872aca1 <+145>: pop %ebx 0x0872aca2 <+146>: pop %ebp 0x0872aca3 <+147>: ret Examining a build decoder made, however -- gcc 5.2.1, Ubuntu flavor -- I get this assembly: (rr) disas Dump of assembler code for function js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int): ... 0x0874aad9 <+57>: xor %eax,%eax 0x0874aadb <+59>: jmp 0x874aae7 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+71> 0x0874aadd <+61>: lea 0x0(%esi),%esi 0x0874aae0 <+64>: add $0x1,%eax 0x0874aae3 <+67>: cmp %ecx,%eax 0x0874aae5 <+69>: je 0x874ab20 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+128> => 0x0874aae7 <+71>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x0874aaec <+76>: jne 0x874aae0 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+64> 0x0874aaee <+78>: fildl (%edx,%eax,8) 0x0874aaf1 <+81>: mov $0x14b,%ecx 0x0874aaf6 <+86>: fstpl (%edx,%eax,8) 0x0874aaf9 <+89>: lea -0x20fbc0(%ebx),%edx 0x0874aaff <+95>: lea -0x24537e(%ebx),%eax 0x0874ab05 <+101>: call 0x8074f6a <MOZ_ReportAssertionFailure(char const*, char const*, int)> 0x0874ab0a <+106>: movl $0x0,0x0 0x0874ab14 <+116>: ud2 0x0874ab16 <+118>: lea 0x0(%esi),%esi 0x0874ab19 <+121>: lea 0x0(%edi,%eiz,1),%edi 0x0874ab20 <+128>: or $0x1,%esi 0x0874ab23 <+131>: mov $0x1,%eax 0x0874ab28 <+136>: mov %esi,-0x10(%edx) 0x0874ab2b <+139>: pop %ebx 0x0874ab2c <+140>: pop %esi 0x0874ab2d <+141>: pop %ebp 0x0874ab2e <+142>: ret 0x0874ab2f <+143>: nop Look at the code starting with cmpl. In my build we test for int32 tag and if it's not int32 continue to the fildl. Same in decoder's build. Now in mine, we have a load-int32-as-double-and-store-back-as-double, then we compare type-tag to JSVAL_TAG_DOUBLE and jump to the next loop iteration if it's a double. But in decoder's build we a fstpl that I believe stores the int32 we just loaded back into the element, encoded as double, then (plus the prior mov) we have code performing a MOZ_RAF(location, assert-expression, line of assertion). Cracktastic. Soooooooooo. Compiler bug, or compiler taking advantage of UB based on strict aliasing or somesuch? The odds are extraordinarily good for either possibility.
Assignee | ||
Comment 6•8 years ago
|
||
Best guess right now is that the compiler sees isInt32() was true, so it knows |data.s.tag == JSVAL_TAG_INT32|. It sees after that an assignment to |data.asDouble|, which has different type from the other, so by C++ rules can't affect the value of the other. Then it assumes the |data.s.tag <= JSVAL_TAG_CLEAR| evaluates to false, like the presumed value would assume, and we fall into the assert. I'm attempting to work around this by doing the double-assignment using memcpy. First question is how much it pessimizes asm in my compiler -- if it doesn't pessimize, we can see if it fixes decoder's compiler. To summarize: we do very dodgy things that violate the C++ type system and memory model, almost certainly. Compiler presumes C++ type system not violated by our code and generates undesirable code that breaks. Hilarity.
Assignee | ||
Comment 7•8 years ago
|
||
Eeugh: 0x0872af87 <+87>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x0872af8c <+92>: jne 0x872af80 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+80> 0x0872af8e <+94>: fildl (%edx,%eax,8) 0x0872af91 <+97>: fstpl -0x28(%ebp) 0x0872af94 <+100>: mov -0x24(%ebp),%edi 0x0872af97 <+103>: mov -0x28(%ebp),%esi 0x0872af9a <+106>: mov %edi,0x4(%edx,%eax,8) 0x0872af9e <+110>: cmpl $0xffffff80,0x4(%edx,%eax,8) 0x0872afa3 <+115>: mov %esi,(%edx,%eax,8) 0x0872afa6 <+118>: ja 0x872b020 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+240> 0x0872afa8 <+120>: mov -0x1c(%ebp),%esi 0x0872afab <+123>: add $0x1,%eax 0x0872afae <+126>: mov 0x4(%esi),%ecx 0x0872afb1 <+129>: cmp %ecx,%eax 0x0872afb3 <+131>: jb 0x872af87 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+87> Swapping in a |memcpy(reinterpret_cast<char*>(&data.asDouble), &d, sizeof(d));| for the existing |data.asDouble = d;| results in that assembly, which appears to spill the int-as-double back onto the stack, read it back in two ops, then write it into the *actual* desired location in another two ops. Grotesque. Compiling it for 64-bit doesn't do much better: 0x0000000000a7da45 <+85>: test %r8d,%r8d 0x0000000000a7da48 <+88>: jne 0xa7da60 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+112> 0x0000000000a7da4a <+90>: jmp 0xa7daa5 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+181> 0x0000000000a7da4c <+92>: nopl 0x0(%rax) 0x0000000000a7da50 <+96>: add $0x1,%rdx 0x0000000000a7da54 <+100>: mov %r8d,%ecx 0x0000000000a7da57 <+103>: add $0x8,%rax 0x0000000000a7da5b <+107>: cmp %rdx,%rcx 0x0000000000a7da5e <+110>: jbe 0xa7daa2 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+178> 0x0000000000a7da60 <+112>: mov (%rax),%rcx 0x0000000000a7da63 <+115>: mov %rcx,%rdi 0x0000000000a7da66 <+118>: shr $0x2f,%rdi 0x0000000000a7da6a <+122>: cmp $0x1fff1,%edi 0x0000000000a7da70 <+128>: jne 0xa7da50 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+96> 0x0000000000a7da72 <+130>: pxor %xmm0,%xmm0 0x0000000000a7da76 <+134>: cvtsi2sd %ecx,%xmm0 0x0000000000a7da7a <+138>: movsd %xmm0,-0x8(%rbp) 0x0000000000a7da7f <+143>: mov -0x8(%rbp),%rcx 0x0000000000a7da83 <+147>: mov %rcx,(%rax) 0x0000000000a7da86 <+150>: or %r10,%rcx 0x0000000000a7da89 <+153>: cmp %r9,%rcx 0x0000000000a7da8c <+156>: ja 0xa7dae0 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+240> 0x0000000000a7da8e <+158>: mov 0x4(%r11),%r8d 0x0000000000a7da92 <+162>: add $0x1,%rdx 0x0000000000a7da96 <+166>: add $0x8,%rax 0x0000000000a7da9a <+170>: mov %r8d,%ecx 0x0000000000a7da9d <+173>: cmp %rdx,%rcx 0x0000000000a7daa0 <+176>: ja 0xa7da60 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+112> 0x0000000000a7daa2 <+178>: mov -0x10(%rsi),%ecx 0x0000000000a7daa5 <+181>: or $0x1,%ecx That compared to the |data.asDouble = d;| version which is 0x0000000000a7e0d6 <+54>: xor %eax,%eax 0x0000000000a7e0d8 <+56>: movabs $0x8000000000000000,%r9 0x0000000000a7e0e2 <+66>: movabs $0xfff80000ffffffff,%r8 0x0000000000a7e0ec <+76>: jmp 0xa7e0f9 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+89> 0x0000000000a7e0ee <+78>: xchg %ax,%ax 0x0000000000a7e0f0 <+80>: add $0x1,%rax 0x0000000000a7e0f4 <+84>: cmp %rdi,%rax 0x0000000000a7e0f7 <+87>: je 0xa7e150 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+176> 0x0000000000a7e0f9 <+89>: mov (%rsi,%rax,8),%rdx 0x0000000000a7e0fd <+93>: mov %rdx,%rcx 0x0000000000a7e100 <+96>: shr $0x2f,%rcx 0x0000000000a7e104 <+100>: cmp $0x1fff1,%ecx 0x0000000000a7e10a <+106>: jne 0xa7e0f0 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+80> 0x0000000000a7e10c <+108>: pxor %xmm0,%xmm0 0x0000000000a7e110 <+112>: cvtsi2sd %edx,%xmm0 0x0000000000a7e114 <+116>: movq %xmm0,%rdx 0x0000000000a7e119 <+121>: movsd %xmm0,(%rsi,%rax,8) 0x0000000000a7e11e <+126>: or %r9,%rdx 0x0000000000a7e121 <+129>: cmp %r8,%rdx 0x0000000000a7e124 <+132>: jbe 0xa7e0f0 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned long)+80> 0x0000000000a7e126 <+134>: lea 0x4f3f5b(%rip),%rsi # 0xf72088 0x0000000000a7e12d <+141>: lea 0x4b96c3(%rip),%rdi # 0xf377f7 0x0000000000a7e134 <+148>: mov $0x14c,%edx 0x0000000000a7e139 <+153>: callq 0x43207a <MOZ_ReportAssertionFailure(char const*, char const*, int)> 0x0000000000a7e13e <+158>: movl $0x0,0x0 0x0000000000a7e149 <+169>: ud2
Assignee | ||
Comment 8•8 years ago
|
||
This might do the trick, maybe, but we'll have to identify every buggy compiler and explicitly opt it into the memcpy version. Ugh. Anyway, it's the morning, so I'm out now -- someone else push this the rest of the way, all the pertinent facts are here and now it's just drudgework of deciding how to work around compilers.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment 9•8 years ago
|
||
Looks like `data = layout(d);` can generate same assembly as `data.asDouble = d;`, and can avoid the issue, thanks to anba :)
Flags: needinfo?(arai.unmht)
Comment 10•8 years ago
|
||
Ubuntu gcc 5.2.1 + `data = layout(d);` 0x08756357 <+71>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x0875635c <+76>: jne 0x8756350 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+64> 0x0875635e <+78>: fildl (%edx,%eax,8) 0x08756361 <+81>: fstpl (%edx,%eax,8) 0x08756364 <+84>: cmpl $0xffffff80,0x4(%edx,%eax,8) Ubuntu gcc 5.2.1 + `memcpy(reinterpret_cast<char*>(&data.asDouble), &d, sizeof(d));` 0x08755787 <+87>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x0875578c <+92>: jne 0x8755780 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+80> 0x0875578e <+94>: fildl (%edx,%eax,8) 0x08755791 <+97>: fstpl -0x28(%ebp) 0x08755794 <+100>: mov -0x24(%ebp),%edi 0x08755797 <+103>: mov -0x28(%ebp),%esi 0x0875579a <+106>: mov %edi,0x4(%edx,%eax,8) 0x0875579e <+110>: cmpl $0xffffff80,0x4(%edx,%eax,8) debian gcc 4.9.2 + `data.asDouble = d;` 0x086bc7df <+79>: cmpl $0xffffff81,0x4(%edx,%eax,8) 0x086bc7e4 <+84>: jne 0x86bc7d8 <js::ObjectElements::ConvertElementsToDoubles(JSContext*, unsigned int)+72> 0x086bc7e6 <+86>: fildl (%edx,%eax,8) 0x086bc7e9 <+89>: fstpl (%edx,%eax,8) 0x086bc7ec <+92>: cmpl $0xffffff80,0x4(%edx,%eax,8)
Comment 11•8 years ago
|
||
here's patch that works on gcc 5.2.1.
Comment 12•8 years ago
|
||
FWIW I remember seeing this assert when I ran jit-tests on OS X last week with a 32-bit build (Clang 3.9.0).
Comment 13•8 years ago
|
||
Comment on attachment 8805050 [details] [diff] [review] Assign whole layout instead of asDouble member in JS::Value::setDoubleNoCheck. Review of attachment 8805050 [details] [diff] [review]: ----------------------------------------------------------------- Reviewing as requested. The Clang assertion failures I mentioned in comment 12 I can no longer reproduce, must have been unrelated. ::: js/public/Value.h @@ +331,5 @@ > MOZ_ASSERT(isDouble()); > } > > void setDoubleNoCheck(double d) { > + data = layout(d); I would add a brief comment here to keep us from "simplifying" this later. Something like: "Don't assign to data.asDouble to fix a miscompilation with GCC X.Y.Z. See bug 1312488." If it ever shows up again we can go with memcpy I guess.
Attachment #8805050 -
Flags: review+
Comment 14•8 years ago
|
||
thank you for reviewing :) running try https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd25c2f48b9c
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/045cb5724eaf1a34be2748e59ee2ebd353d24f4b Bug 1312488 - Assign whole layout instead of layout.asDouble in JS::Value::setDoubleNoCheck. r=jandem
Assignee | ||
Comment 16•8 years ago
|
||
Someone's going to write and land a testcase that clearly invokes a ConvertElementsToDouble on the elements of an array starting with an int32, right? (i.e. not something goofball like the original testcase that *somehow* triggers that, but exactly why/when it does so is 100% opaque.) Now that we know *some* compilers freak on this, we definitely need to verify it doesn't happen again.
Updated•8 years ago
|
Keywords: leave-open
Comment 17•8 years ago
|
||
the testcase needs to satisfy the following conditions... https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/js/src/jit/IonBuilder.cpp#9828-9837 > bool loadDouble = > unboxedType == JSVAL_TYPE_MAGIC && > barrier == BarrierKind::NoBarrier && > loopDepth_ && > inBounds && > knownType == MIRType::Double && > objTypes && > objTypes->convertDoubleElements(constraints()) == TemporaryTypeSet::AlwaysConvertToDoubles; > if (loadDouble) > elements = addConvertElementsToDoubles(elements);
Comment 18•8 years ago
|
||
Added a testcase, that does dense element access in a loop, with all double elements except the last one that is int32.
Attachment #8805275 -
Flags: review?(jwalden+bmo)
Comment 19•8 years ago
|
||
the testcase hits assertion failure on gcc 5.2.1 with unpatched build. and patched build passes it.
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/045cb5724eaf
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8805275 [details] [diff] [review] Part 2: Add a testcase for ConvertElementsToDouble. Review of attachment 8805275 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/ConvertElementsToDouble-Int32.js @@ +5,5 @@ > + for (let i = 0, len = a.length; i < len; i++) > + x += a[i]; > + return x; > +} > +test([10.1, 10.2, 10.3, 10.4, 10]); assertEq(test(...), 41);
Attachment #8805275 -
Flags: review?(jwalden+bmo) → review+
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f89f06bdb13ae1574dac84a8b56e5d9db4ba6c9 Bug 1312488 - Part 2: Add a testcase for ConvertElementsToDouble. r=jwalden
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8f89f06bdb13
Comment 24•8 years ago
|
||
this is from bug 1304191. affects only firefox52.
Blocks: 1304191
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•