854103 - bug507180.js:31:4 Error: Assertion failed: got "imacro produces incorrect result for obj == any", expected "fail" on linux64 when built with gcc4.7

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=21010620&tree=Try

FAIL - /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js
TEST-UNEXPECTED-FAIL | /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js | --no-ion --always-mjit: /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js:31:4 Error: Assertion failed: got "imacro produces incorrect result for obj == any", expected "fail"
FAIL - /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js
TEST-UNEXPECTED-FAIL | /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js | --no-ion --always-mjit --debugjit: /builds/slave/try-l64-d-00000000000000000000/build/js/src/jit-test/tests/basic/bug507180.js:31:4 Error: Assertion failed: got "imacro produces incorrect result for obj == any", expected "fail"

Comment 1

[Mike Hommey [:glandium]]

Only happens on debug builds, not opt.

Comment 2

[Mike Hommey [:glandium]]

GCC 4.7 can be tested on try with:

--- a/build/unix/mozconfig.linux
+++ b/build/unix/mozconfig.linux
@@ -1,4 +1,4 @@
 . "$topsrcdir/build/mozconfig.common"
 
-CC=/tools/gcc-4.5-0moz3/bin/gcc
-CXX=/tools/gcc-4.5-0moz3/bin/g++
+CC=/tools/gcc-4.7.2-0moz1/bin/gcc
+CXX=/tools/gcc-4.7.2-0moz1/bin/g++

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: minized bug 507180.js

This is a minimal version of bug507180.js that fails for me too.  Note the empty for loop, function call to assert() and the object weirdness all seem to be required.  Also note that this only seems to fail with --args='--no-ion --no-baseline --always-mjit' :(

Comment 4

[Mike Hommey [:glandium]]

It's also happening on PGO builds
https://tbpl.mozilla.org/php/getParsedLog.php?id=21655341&tree=Try

Comment 5

[Trevor Saunders (:tbsaunde)]

So, It seems that js::methodjit::stubs::Equal() returns false and sets argv[-2] to a false jsval correctly, but when we get to js::AssertEq() the first arg is true.  I sort of suspect js/src/methodjit/InvokeHelpers.cpp:883 but don't know teh code well enough to really say if that's a reasonable explanation of what's happening.

I supose another approach we could take is to just disable testing of JM (dvander was suprised we are testing it and apparently its about to be removed) in which case we could just hope its not a compiler bug and move on.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Never inline js::mjit::StubEqualityOp

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> So, It seems that js::methodjit::stubs::Equal() returns false and sets
> argv[-2] to a false jsval correctly, but when we get to js::AssertEq() the
> first arg is true.  I sort of suspect js/src/methodjit/InvokeHelpers.cpp:883
> but don't know teh code well enough to really say if that's a reasonable
> explanation of what's happening.

So, in fact, js::mjit::stubs::Equal is not returning false properly. It returns JSVAL_FALSE instead of false. That is, it forgets to do the toBoolean(). Well, gcc 4.7 makes it forget to do it, which seems to be a case of bad inlining.

This patch works for me locally.

Comment 7

[Mike Hommey [:glandium]]

And it works on try with gcc 4.7.
https://tbpl.mozilla.org/?tree=Try&rev=d12149b3030d

PGO still building, but at this point there's no reason it should fail.
https://tbpl.mozilla.org/?tree=Try&rev=bc85d9bb84c9

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 736203 [details] [diff] [review]
Never inline js::mjit::StubEqualityOp

All green on try with gcc 4.7.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 736203 [details] [diff] [review]
Never inline js::mjit::StubEqualityOp

As discussed on irc, this probably isn't the right fix, the root cause is somewhere else.

Comment 10

[Mike Hommey [:glandium]]

The actual problem is between INLINE_STUBCALL and JaegerInterpoline. Essentially, all stub calls are treated as if they returned void *, and that void * ends up as returnReg in js_InternalInterpret. The problem is that on 64-bits platforms, stubs returning a JSBool, may leave garbage in the 32 upper bits, which end up in that void *.

Now, the question is, is REJOIN_PUSH_BOOLEAN the only rejoin value where the called stub returns a JSBool?

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

If this is only a problem on mozilla-central, we should just stop testing JM on tbpl, as we don't expect to ship it enabled with Firefox 23.

Comment 12

[Mike Hommey [:glandium]]

(In reply to David Anderson [:dvander] from comment #11)
> If this is only a problem on mozilla-central, we should just stop testing JM
> on tbpl, as we don't expect to ship it enabled with Firefox 23.

If we don't have test coverage of all the possible combinations that can lead to such problems, which can theoretically happen with any compiler, we may have a hidden bug on any release using JM.

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: ignore the high bits of returnReg on x86_64 when it actually holds a bool

I wrote this before you commented, and so figure I might as well post it.  Also we probably don't need to, but I guess we might want to take it on branches...

p.s. sorry its not diff -U8 -p it was written on a machine I don't normally use and I forgot until just now.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to David Anderson [:dvander] from comment #11)
> If this is only a problem on mozilla-central, we should just stop testing JM
> on tbpl, as we don't expect to ship it enabled with Firefox 23.

filed bug 860915 to stop testing JM since I assume you'll want to do that at some point if not now.

Comment 15

[Mike Hommey [:glandium]]

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

Sorry, I appear to have forgotten to hg qref -u

Comment 16

[Phil Ringnalda (:philor)]

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