445332 - TT: Failing array acceptance testcases

Status: VERIFIED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Brent Baker]

The following failures where introduced with changeset 486:ff804648d916

If you change the order in which the testcases execute they will pass, possibly pointing to a memory issue? Example: If you move the "exhaustive slice test 4" in e15_4_4_10 above the "exhaustive slice test 3" then all tests will pass. This is the same in all of the failing testcases and platforms.


testcase: ecma3/Array/e15_4_4_10.as  (exhaustive slice test 4)
platform: Windows Release 

testcase: ecma3/Array/e15_4_4_10.as  (exhaustive slice test 2)
platform: Windows Release -interp, Mac OSX Release -interp

testcase: ecma3/Array/slice.as (exhaustive slice test 2)
platform: Mac Release –interp

testcase: ecma3/Array/splice1.as (exhaustive splice w/no optional args 1)
platform: Windows Mobile Release ARM and THUMB

Comment 1

[Brent Baker]

Pushed change to expect the above tests to fail on respective platforms (503:f161b485027e)

Comment 2

[Edwin Smith]

I beleive change 490 (allocate rstack) was an injection of a lot of weird behavior and I think 

Comment 3

[Edwin Smith]

... that changeset 510 is the most likely change that resolved lots of weird problems. 

as of build 515:
* e15_4_4_10 consistenly passes all on win-release-jit
* e15_4_4_10 consistently fails on win-release-interp, but passes without TT_NEW_STRINGS.  when it fails with TT_NEW_STRINGS enabled, the message is:

     actual result: 6,7,8,9,0
   expected result: 6,7,8,9,0

probably a bug in string comparison; the strings are both correct.

Comment 4

[Michael Daumling]

Attachment: Bug fix

Nice bug...

There is an optimization in concat() where I check if rightStr already has been appended in-place to leftStr. If yes, only a dependent string is created, wrapping leftStr plus the in-place data corresponding to rightStr. If is so happens that leftStr is full, but the next pattern in memory just happens to match rightStr, the dependent string is created as well.

In the concrete example, leftStr was "6,7,8,9," and rightSTr was "0". And, since MMgc allocates on 16-byte boundaries, the lowest byte of the address that MMgc happened to place next to the string buffer, ended in 0x30 == "0". This caused concat() to wrongly assume that rightStr already was present as an appended part of leftStr ("6,7,8,9,0").

The fix is simply to check if leftStr has been filled already...

No memory overwrites were involved. Just a dependent String pointing to a buffer whose contents can change...

Comment 5

[Brent Baker]

Attachment: Remove expected failures

When path 330028 is pushed this should also be pushed. This removes the expected failures from the acceptance run. This has been tested with the patch on windows, mac, linux and windows mobile ARM|THUMB

Comment 6

[Michael Daumling]

Could someone, please, push this patch so the bug can be closed? Thank you.

Comment 7

[Edwin Smith]

if you merge & clean it up, and push to the sandbox, i'll push to tamarin-tracing

sandbox repo:
http://hg.mozilla.org/users/brbaker_adobe.com/tamarin-tracing/

Comment 8

[Brent Baker]

I have pushed the patch into the sandbox:
Changed by: Brent Baker <[e-mail]>
Changed at: Tue 22 Jul 2008 10:41:46
Revision: 559:75668a54b34f

Changed files:

    * core/StringObjectFW.cpp
    * test/acceptance/testconfig.txt

Comments:

Bug 445332 check if leftStr has been filled already in concat() (p=[e-mail])


http://tamarin-builds.mozilla.org/tamarin-tracing-sandbox/waterfall

Comment 9

[Edwin Smith]

pushed to TT

Comment 10

[Brent Baker]

All testscases are back to passing as of 520:75668a54b34f.

Marking bug as verified.