830676 - YarrJIT is broken on Win64 after landing bug 820124

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

Win64 is tier-3 platform, so we don't need backout bug 820124.


https://tbpl.mozilla.org/php/getParsedLog.php?id=18799707&tree=Mozilla-Inbound&full=1

TEST-UNEXPECTED-FAIL | jit_test.py --no-jm | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --ion-eager | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-jm --no-ti| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-ti| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-jm| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-ti -a -d| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -d | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -a | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -a -d | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\basic\bug593663-regexp.js:24:0 Error: Assertion failed: got "", expected "my money"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-ti| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-jm | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --ion-eager | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-jm --no-ti| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-ti -a -d| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion --no-jm| e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -a | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -a -d | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
TEST-UNEXPECTED-FAIL | jit_test.py --no-ion -d | e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js: e:\builds\moz2_slave\m-in-w64\build\js\src\jit-test\tests\debug\Frame-onStep-08.js:26:0 Error: Assertion failed: got "", expected "BBBB"
Only displaying first 20 of 40 failures - View log.

Comment 1

[Makoto Kato [:m_kato]]

This is Win64 ABI issue.  Win64 code cannot get 2nd return value...

MatchResult is 2 size_t value, if Unix x86-64 ABI, we can get value by registers (RAX and RDX) only.  But Win64 ABI, MatchResult should use pointer instead of...

Comment 2

[Sean Stangl [:sstangl]]

This is Bug 826588, which was made as part of the Yarr import, and exposed first by Bug 808245. Yarr internals assume the presence of a second return register -- to avoid changing the JIT too much, it may be easier to create a Win64-specific shim that transfers from the two fake return registers into the final location expected by the ABI, which is apparently an implicit first argument.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 705257 [details] [diff] [review]
fix

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

r+ with comments addressed.

::: js/src/yarr/MatchResult.h
@@ +34,5 @@
> +// no 2nd return register on Win64 ABI
> +typedef uint32_t MatchResultSizeT;
> +#else
> +typedef size_t MatchResultSizeT;
> +#endif

In our engine, strings are limited to a length of 2^28, so we should always be able to use |int|.

This patch looks good, but instead of using MatchResultSizeT, let's just always use int. (This may also remove the distinction between MatchResult and MatchPair, but that can be done later.)

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Sean Stangl from comment #4)
> Comment on attachment 705257 [details] [diff] [review]
> fix
> 
> Review of attachment 705257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: js/src/yarr/MatchResult.h
> @@ +34,5 @@
> > +// no 2nd return register on Win64 ABI
> > +typedef uint32_t MatchResultSizeT;
> > +#else
> > +typedef size_t MatchResultSizeT;
> > +#endif
> 
> In our engine, strings are limited to a length of 2^28, so we should always
> be able to use |int|.
> 
> This patch looks good, but instead of using MatchResultSizeT, let's just
> always use int. (This may also remove the distinction between MatchResult
> and MatchPair, but that can be done later.)

UNIX x86-64 implementation still uses 2 registers.  (struct MatchResult has 2 size_t.)  So should I change YarrJIT code like Win64 on UNIX x86-64 ABI, too?

Comment 6

[Sean Stangl [:sstangl]]

(In reply to Makoto Kato from comment #5)
> UNIX x86-64 implementation still uses 2 registers.  (struct MatchResult has
> 2 size_t.)  So should I change YarrJIT code like Win64 on UNIX x86-64 ABI,
> too?

That should be safe, yeah.

Comment 7

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/mozilla-central/rev/30fcaabe235a

Comment 8

[Sean Stangl [:sstangl]]

(In reply to Makoto Kato from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/30fcaabe235a

Thank you for fixing this.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: warning fix

this made my JS builds rather noisy

Comment 10

[[e-mail]]

Comment on attachment 705257 [details] [diff] [review]
fix

>+    // no way to use int128_t as return value on Win64 ABI

Is it possible to use __m128* instead?

http://msdn.microsoft.com/en-us/library/7572ztz4.aspx

"Unless you're returning __m128, __m128i, __m128d, floats, and doubles, which are returned in XMM0..."

Comment 11

[Makoto Kato [:m_kato]]

(In reply to [e-mail] from comment #10)
> Comment on attachment 705257 [details] [diff] [review]
> fix
> 
> >+    // no way to use int128_t as return value on Win64 ABI
> 
> Is it possible to use __m128* instead?
> 
> http://msdn.microsoft.com/en-us/library/7572ztz4.aspx
> 
> "Unless you're returning __m128, __m128i, __m128d, floats, and doubles,
> which are returned in XMM0..."

It is expensive to use XMM register and mov XMM -> GPRs.  Also, Sean says string max length is 2^28, so it is necessary to use 64bit register for start and end.

Comment 12

[Ed Morley [:emorley]]

Still broken it seems:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19118089&tree=Firefox

Comment 13

[Luke Wagner [:luke]]

The patch in comment 9 fixes the most common warnings, but there are several more warnings in YarrJIT.cpp at each use of WTF::notFound; perhaps we could change it's type to be 'int'?

Comment 14

[Sean Stangl [:sstangl]]

Comment on attachment 706287 [details] [diff] [review]
warning fix

Handled by Bug 834762. Sorry for stealing -- the patch needed a few changes. I tried to push it with the changes, but the tree is closed.

Let's keep this bug focused on the Win64 issues (which may still be present?).

Comment 16

[[e-mail]]

(In reply to Makoto Kato from comment #11)
> (In reply to comment #10)
> > Is it possible to use __m128* instead?
> It is expensive to use XMM register and mov XMM -> GPRs.
OK fair enough thanks.

Comment 17

[Sean Stangl [:sstangl]]

Comment on attachment 705257 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Yarr import (740015) + MatchOnly mode (808245).
User impact if declined: Win64 randomly crashes during regexp operation.
Testing completed (on m-c, etc.): m-c, fuzzing.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Note that landing should also be accompanied by the patch that fixes the warnings, and that the patch pushed to aurora should be the one pushed to m-c, which fixes small nits from review.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 705257 [details] [diff] [review]
fix

Approving for uplift - along with warning fix and the nits address from review.

Comment 19

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ed750ce8e6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c559854783b