700983 - linux-arm-deep mops breakage

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

See for example:

http://tamarin-builds.mozilla.org/tamarin-redux/builders/linux-arm-deep/builds/397/steps/Testsuite_Release-softfloat-deep/logs/stdio

where it says:

FAILURES:
  mops/mops.abc_ : Read test double w/ min sized = false FAILED! expected: true
  mops/mops.abc_ : Write test double w/ min sized = false FAILED! expected: true
END FAILURES

brbaker adds:

happened between 6655 and 6660 (6654 passed, 6660 was the next build with the failure)

Comment 1

[Brent Baker]

This was injected with changeset:


changeset:   6657:5532f94bc66b
user:        Lars T Hansen <[e-mail]>
date:        Mon Oct 17 13:02:28 2011 +0200
files:       core/ByteArrayGlue.cpp core/ByteArrayGlue.h
description:
Fix 692042 - Optimize ByteArray: readByte, readInt, readDouble, etc (r=fklockii)


This should be resolved before the integration otherwise we will be injecting a bug into MOPS on arm

Comment 2

[Lars T Hansen]

Noted.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Changing ByteArrayObject::readDouble() to its prior (trivial) implmentation,

  return m_byteArray.ReadDouble();

makes the test succeed.

So the test is exposing some bug in Lars's rewrite of ByteArrayObject::readObject.  (There may be analogous bugs elsewhere in the changeset 6657:5532f94bc66b patch.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

I hacked ByteArrayGlue.cpp and some associated files to first peek at 8 bytes at the cursor in the bytearray, convert via the old m_byteArray.ReadDouble code, run Lars's revised code, and compare the results.  If the results did not match, it prints the 8 bytes (b_orig), the value computed by Lars's code (u.dval), and the value computed m_byteArray.ReadDouble (val).

Here are the first three non-matches printed by the hack:

b_orig: [174, 71, 65, 64, 72, 225, 122, 20]
u.dval: 34.56
   val: 5.11014e-210
b_orig: [174, 71, 69, 64, 72, 225, 122, 20]
u.dval: 42.56
   val: 5.11014e-210
b_orig: [174, 71, 73, 64, 72, 225, 122, 20]
u.dval: 50.56
   val: 5.11014e-210

I'll attach the whole list shortly.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Log N v1: list of non-matches when comparing old and new code

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

#tamarin IRC recap:

Felix eventually had the insight to directly compared the byte order of val and u.dval in one of the failing cases.

bad version: {0x147ae148, 0x404147ae}
"good" path: {0x404147ae, 0x147ae148}

So clearly the words have been swapped.

Lars says its because "the little-endian native-endian non-unaligned-access case is wrong, it swaps the words.  basically that case should be a copy"

Felix reviewed and noted that this seems to makes sense, as the DataIO.h does not go through the swapping path when the endiannesses match up.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v1: fix little-endian endianness-matching case

(will write up a regression test, and look into why our current test suite failed to catch this, separately.)

Comment 8

[Lars T Hansen]

Comment on attachment 573526 [details] [diff] [review]
patch P v1: fix little-endian endianness-matching case

comment changes are wrong and need to be undone.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(this fix does not suffice and will cause ByteArray acceptance failures.  I suspect there's probably a matching bug in writeDouble.  Investigating.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v2: fix little-endian endianness-matching case

This probably explains why the previous code got through our acceptance tests: two errors were conspiring to mask the error.

Not quite sure how best to address this in our tests.  One option would be to bite the bullet and make a test that verifies the raw byte representation within the byte array, rather than just checking that values are round-trippable.

Comment 11

[Lars T Hansen]

I think resorting to a couple of specific tests using sequences of known bytes is right.  We're using IEEE arithmetic everywhere so the byte sequences will be invariant.

If we can get PPC testing back on line we will at least test little and big endian and aligned-access and unaligned-access (even if not in all combinations); the only thing we won't test is reversed word order but thankfully that additional wrinkle is not hard.

Comment 12

[Tamarin Bot]

changeset: 6723:3d571bb528d6
user:      Felix S Klock II <[e-mail]>
summary:   Bug 700983: ByteArray read/writeDouble fix le endian match case (r=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/3d571bb528d6

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

This bug is fixed, but we need to figure out a strategy for testing big-endian targets.

(My attempts to use rosetta to emulate a big-endian PPC target on my laptop was not successful.)

I'm not going to bother making a regression test until we have that, because I won't be able to test-the-test sensibly.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #13)
> This bug is fixed, but we need to figure out a strategy for testing
> big-endian targets.

Filed as Bug 708221.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: text file R: failures with rosetta.

(In reply to Felix S Klock II from comment #13)
> (My attempts to use rosetta to emulate a big-endian PPC target on my laptop
> was [sic] not successful.)

For the record, attached are the acceptance failures on an Rosetta PPC build atop fklockii-iMac (a 2.66 Ghz Core i5 running Mac OS X 10.6.8).