1243810 - Crash [@ (anonymous namespace)::SimdToExpr] with gcc 4.7 only

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 211a4c710fb6 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --ion-extra-checks --ion-eager):

function asmLink(global, imp, buffer) {
    "use asm";
    var toF = global.Math.fround;
    var f4 = global.SIMD.Float32x4;
    var f4load = f4.load;
    function update(timeDelta) {
        timeDelta = toF(timeDelta);
        var newVelx4 = f4(0.0,0.0,0.0,0.0);
        newVelx4 = f4load(u8, (i & mk4) + maxBirdsx4);
    }
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x080e5f3a in (anonymous namespace)::SimdToExpr (type=type@entry=<incomplete type>, op=js::Fn_load) at js/src/asmjs/AsmJS.cpp:2599
2599	        ENUMERATE(F32x4, FORALL_FLOAT32X4_ASMJS_OP, F32CASE)
#0  0x080e5f3a in (anonymous namespace)::SimdToExpr (type=type@entry=<incomplete type>, op=js::Fn_load) at js/src/asmjs/AsmJS.cpp:2599
#1  0x081014df in writeSimdOp (op=<optimized out>, simdType=<incomplete type>, this=0xffffa6dc) at js/src/asmjs/AsmJS.cpp:2768
#2  CheckSimdLoad (f=..., call=0xf7ab3798, opType=<incomplete type>, op=op@entry=js::Fn_load, type=type@entry=0xffffa598) at js/src/asmjs/AsmJS.cpp:5198
#3  0x081043c4 in CheckSimdOperationCall (f=..., call=<optimized out>, global=0xf7a93070, type=type@entry=0xffffa598) at js/src/asmjs/AsmJS.cpp:5343
#4  0x0810477a in CheckUncoercedCall (f=..., expr=expr@entry=0xf7ab3798, type=0xffffa598) at js/src/asmjs/AsmJS.cpp:5407
#5  0x080fcc6c in CheckExpr (f=..., expr=expr@entry=0xf7ab3798, type=type@entry=0xffffa598) at js/src/asmjs/AsmJS.cpp:6109
#6  0x08101a45 in CheckAssignName (f=..., lhs=lhs@entry=0xf7ab3748, rhs=rhs@entry=0xf7ab3798, type=type@entry=0xffffa61c) at js/src/asmjs/AsmJS.cpp:3853
#7  0x08101cb2 in CheckAssign (f=..., assign=<optimized out>, type=0xffffa61c) at js/src/asmjs/AsmJS.cpp:3898
#8  0x080fcb0c in CheckExpr (f=..., expr=<optimized out>, type=type@entry=0xffffa61c) at js/src/asmjs/AsmJS.cpp:6101
#9  0x0810479c in CheckAsExprStatement (expr=<optimized out>, f=...) at js/src/asmjs/AsmJS.cpp:6146
#10 CheckExprStatement (f=..., exprStmt=<optimized out>) at js/src/asmjs/AsmJS.cpp:6156
#11 0x08104abc in CheckStatement (f=..., stmt=<optimized out>) at js/src/asmjs/AsmJS.cpp:6597
#12 0x0810b155 in CheckFunction (m=...) at js/src/asmjs/AsmJS.cpp:6717
#13 0x0810b41e in CheckFunctions (m=...) at js/src/asmjs/AsmJS.cpp:6766
#14 0x0810b619 in CheckModule (cx=cx@entry=0xf7a72040, parser=..., stmtList=stmtList@entry=0xf7ab3168, moduleObj=moduleObj@entry=..., time=time@entry=0xffffb3c0, slowFuncs=slowFuncs@entry=0xffffb3ec) at js/src/asmjs/AsmJS.cpp:6977
#15 0x0810b9b5 in js::CompileAsmJS (cx=0xf7a72040, parser=..., stmtList=stmtList@entry=0xf7ab3168, validated=validated@entry=0xffffb49c) at js/src/asmjs/AsmJS.cpp:8245
[...]
#38 main (argc=7, argv=0xffffcf74, envp=0xffffcf94) at js/src/shell/js.cpp:7000
eax	0x9484ff4	155734004
ebx	0x9484ff4	155734004
ecx	0x2	2
edx	0xf7a40708	-140245240
esi	0xf7ab3798	-139774056
edi	0xffffa6dc	-22820
ebp	0xffffa806	4294944774
esp	0xffffa4a0	4294943904
eip	0x80e5f3a <(anonymous namespace)::SimdToExpr(js::SimdType, js::SimdOperation)+170>
=> 0x80e5f3a <(anonymous namespace)::SimdToExpr(js::SimdType, js::SimdOperation)+170>:	sub    -0x139f0b0(%ebx,%edx,4),%eax
   0x80e5f41 <(anonymous namespace)::SimdToExpr(js::SimdType, js::SimdOperation)+177>:	jmp    *%eax


This is a recent regression on nightly and requires a build with gcc 4.7 as far as I can tell (exact version was gcc (Ubuntu/Linaro 4.7.3-2ubuntu1~12.04) 4.7.3).

Comment 1

[Christian Holler (:decoder)]

Needinfo from :bbouvier per IRC discussion.

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

I'm using gcc 5.2.1. Let's try a few easy patches.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1-dontinline.patch

Can you do me a favor and test these patches on your installation, please? Each of them needs to be applied separately, then you need to see whether the segfault triggers again.

Hopefully, this one should be enough, but well, compilers gods are mysterious concepts.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2-simdop-storageclass.patch

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3-simdtype-storageclass.patch

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4-both-storageclass.patch

It is expected that if patch 2 or patch 3 fix the issue, it will also be solved by this patch, in case it won't be needed to test.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

cc'ing Jakob because SIMD, Luke because another possible instance of wrong inlining of enum classes.

Comment 9

[Luke Wagner [:luke]]

bbouvier: can you raise this again in the other bug where we were asking to disable these faulty gcc versions?

Comment 10

[Christian Holler (:decoder)]

Comment on attachment 8714293 [details] [diff] [review]
1-dontinline.patch

Bug still reproduces with this patch (patch 1).

Comment 11

[Christian Holler (:decoder)]

Comment on attachment 8714294 [details] [diff] [review]
2-simdop-storageclass.patch

This patch (patch 2) fixes the bug.

Comment 12

[Christian Holler (:decoder)]

Comment on attachment 8714295 [details] [diff] [review]
3-simdtype-storageclass.patch

Bug still reproduces with this patch (patch 3).

Comment 13

[Christian Holler (:decoder)]

Comment on attachment 8714296 [details] [diff] [review]
4-both-storageclass.patch

This patch (patch 4) fixes the bug.

Comment 14

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> cc'ing Jakob because SIMD, Luke because another possible instance of wrong
> inlining of enum classes.

Is this a GCC bug? Is there a GCC bug number?

I think removing the storage class from SimdType is fine if that works around the issue.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #14)
> Is this a GCC bug? Is there a GCC bug number?

Yes, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64037

Hopefully we will drop support for GCC 4.7 soon, bug 1175546.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Change storage class of SimdOperation to uint16_t

Christian, does this one fix your crash?

If so, Jakob, can you review it, please?

I'm opening a follow-up bug to make sure we don't forget about these enums which could be byte-sized but aren't because of this gcc bug.

Comment 17

[Christian Holler (:decoder)]

Comment on attachment 8714686 [details] [diff] [review]
Change storage class of SimdOperation to uint16_t

Unfortunately, this patch doesn't fix the issue for me.

Comment 18

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8714686 [details] [diff] [review]
Change storage class of SimdOperation to uint16_t

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

Looks good.

Could you add a reference to the GCC bug that jandem gave above?

I think it would also be OK to not have a storage class at all, but either way is fine.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the review: i've entirely removed the storage class, as having uint16_t didn't fix the issue.

Landing, because it's a compiler's bug, and other similar compilers bugs have landed without sec-approval.

https://hg.mozilla.org/integration/mozilla-inbound/rev/feae97c5575f

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Landed on central a while ago!

https://hg.mozilla.org/mozilla-central/rev/feae97c5575f

Comment 21

[Eric Faust [:efaust]]

Benjamin, looks like beta 47 is built with 4.7. Is it worth uplifting this fix?

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Eric Faust [:efaust] from comment #21)
> Benjamin, looks like beta 47 is built with 4.7. Is it worth uplifting this
> fix?

No I don't think so: SIMD is Nightly-only, and this is touching SIMD-only paths in asm.js.

Comment 23

[Eric Faust [:efaust]]

You heard the man. WONTFIX 47.