YarrJIT is broken on Win64 after landing bug 820124

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

({regression})

Trunk
mozilla21
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20 fixed, firefox21 fixed)

Details

Attachments

(2 attachments)

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.
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...
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.
Posted patch fixSplinter Review
Attachment #705257 - Flags: review?(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.)
Attachment #705257 - Flags: review?(sstangl) → review+
(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?
(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.
https://hg.mozilla.org/mozilla-central/rev/30fcaabe235a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Makoto Kato from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/30fcaabe235a

Thank you for fixing this.
Posted patch warning fixSplinter Review
this made my JS builds rather noisy
Attachment #706287 - Flags: review?(sstangl)
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..."
(In reply to neil@parkwaycc.co.uk 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.
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'?
Depends on: 834762
Attachment #706287 - Flags: review?(sstangl) → review+
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?).
Attachment #706287 - Flags: review+
Duplicate of this bug: 829738
(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 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.
Attachment #705257 - Flags: approval-mozilla-aurora?
Comment on attachment 705257 [details] [diff] [review]
fix

Approving for uplift - along with warning fix and the nits address from review.
Attachment #705257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.