Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test suite failure with GHCJS-7.10.3 #8

Closed
roelvandijk opened this issue Sep 26, 2016 · 9 comments
Closed

Test suite failure with GHCJS-7.10.3 #8

roelvandijk opened this issue Sep 26, 2016 · 9 comments

Comments

@roelvandijk
Copy link
Contributor

You'll first need the commits associated with #7.

cabal configure --ghcjs -fdev --enable-tests -finstrumented -f-integer-gmp && cabal build && dist/build/tests/tests

...

Linking dist/build/tests/tests.jsexe (Consistency,Main,Vectors.FNV,Vectors.SipHash)
Assertions: On
Getting bytes from Float... OK
Getting bytes from Double...bytesDouble: -2.1006827528635512e-88 /= (136,171,127,215,173,186,190,170)

@roelvandijk
Copy link
Contributor Author

It appears to be a problem with the endianness, but on the level of 32-bit words.

The test suite expects:
(173,186,190,170,136,171,127,215)
but gets
(136,171,127,215,173,186,190,170)

Note how the first and last 4 bytes are swapped.

This appears to be offending code:

doubleToWord :: Double -> Word64
doubleToWord x = runST (castViaSTArray x)

castViaSTArray :: (MArray (STUArray s) a (ST s),
                   MArray (STUArray s) b (ST s)) => a -> ST s b
castViaSTArray x = newArray (0 :: Int,0) x >>= castSTUArray >>= flip readArray 0

I'll try to further pinpoint the problem.

@roelvandijk
Copy link
Contributor Author

roelvandijk commented Sep 26, 2016

@luite Do you have any idea what's happening here? Could it be related to GHCJS's representation of MutableByteArray#?

For reference: I am getting different results running the code with GHC-7.10.3 and with GHCJS-7.10.3.

@jberryman
Copy link
Owner

That hack for extracting bytes from Double may need to change regardless. I'm curious if it even works consistently without --ghcjs, on 32 and 64-bit platforms. I guess it depends on how writeDoubleArray# is implemented.

@roelvandijk
Copy link
Contributor Author

roelvandijk commented Sep 27, 2016

Are you interested in a GHCJS specific conversion? I could try and write one using typed arrays.

But I agree that it would be nice to have a slower, but stable, conversion from Double to bytes. Maybe take a look at the solutions used by various binary serialisation libraries?

@jberryman
Copy link
Owner

I just want something that works consistently across all platforms. Yep, that's a good idea. It looks like using decodeFloat would be a good approach, but we probably also need to get Integer to hash consistently with and without integer-gmp too, I think.

@jberryman
Copy link
Owner

@roelvandijk I haven't used ghcjs at all and just digging into these tickets this weekend. Is this flag for integer-gmp the most convenient way to compile with ghcjs? I think instead we should have an impl(ghcjs) block in the cabal file which disables integer-gmp, but googling around briefly I'm not sure if that's fully supported (it might cause issues on hackage?)

@luite
Copy link

luite commented Nov 5, 2016

Hmm, looking back now, I wonder if this specific problem is caused by some endianness mixups in GHCJS related to 64 bit load/store ops (which are executed as two 32 bit ops on a typed array).

Unfortunately the endianness of the underlying platform is not specified by JS, but GHCJS assumes little endian now because that's by far the most likely one in practice for things running JS (all x86(-64) and Android/iOS ARM devices are little endian).

I might be able to fix this one, but I don't think correctness can be guaranteed on more exotic targets. decodeFloat is definitely safer. GHCJS uses a similar approach here, going through a typed array, but has a fallback for unexpected endianness.

For decodeFloat, GHCJS should always produce the same value as GHC, which the exception of where decodeFloat itself is unspecified (NaN/infinite values): JS engines use the NaN space for other things, and often map all NaN values to a single one. We can't even observe "negative NaN" in JS (see decodeFloat (1/0) vs decodeFloat (-1/0)).

@jberryman
Copy link
Owner

@luite thanks for commenting. I'm going to hash by way of decodeFloat, but I discovered this issue with Word64 serialization affects some of my other instances (e.g. ByteString). I'm not sure this has to do with endianness per se, nor do I think ghcjs is necessarily doing something wrong here; I think the issue is just that I can't make assumptions about how a Word64 is stored on a 32-bit platform.

I suppose the best thing for ghcjs to be doing is matching the same behavior as GHC on 32-bit x86. I need to getter a docker setup for developing and testing this in 32-bit mode, and I'll open a ticket if it looks like it's doing something different than 32-bit ghc, so you can have the discussion there.

I'm excited about the prospects of being able to compare hashes in JS and machine code on client/server! Pretty cool stuff.

@jberryman
Copy link
Owner

Thanks again for your help! Version 2.0.0 just uploaded to hackage should build and work correctly with ghcjs, without any need to pass the -f-integer-gmp flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants