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)

x86
Linux
defect
Not set
critical

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)

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.
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [fuzzblocker] [jsbugmon:bisect] → [fuzzblocker] [jsbugmon:]
Fallout from bug 1243617 maybe?
Flags: needinfo?(arai.unmht)
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)
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)
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.
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.
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
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: nobody → jwalden+bmo
Status: NEW → ASSIGNED
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)
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)
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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/045cb5724eaf1a34be2748e59ee2ebd353d24f4b
Bug 1312488 - Assign whole layout instead of layout.asDouble in JS::Value::setDoubleNoCheck. r=jandem
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.
Keywords: leave-open
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);
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)
the testcase hits assertion failure on gcc 5.2.1 with unpatched build. and patched build passes it.
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+
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.