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

hashable 1.2.0.x segfaults with ByteString on 32bit Linux, GHC 7.0.4 #52

Closed
kazu-yamamoto opened this issue Jan 8, 2013 · 14 comments · Fixed by #53
Closed

hashable 1.2.0.x segfaults with ByteString on 32bit Linux, GHC 7.0.4 #52

kazu-yamamoto opened this issue Jan 8, 2013 · 14 comments · Fixed by #53

Comments

@kazu-yamamoto
Copy link

On 32bit Linux, hashing ByteString causes segfault:

% ghci -XOverloadedStrings

:m Data.Hashable
:m +Data.ByteString.Char8
hash ("/" :: ByteString)
zsh: segmentation fault ghci -XOverloadedStrings

This bug introduced some between 1.1.2.5 and 1.2.0.0. This bug does not exist on 64bit Linux.

@kazu-yamamoto
Copy link
Author

The master branch still causes segfault. I'm using GHC 7.0.4 on a 32bit Linux machine.

This bug exists in compiled code, too. Actually, I found this bug as (complied) Mighty caused segfault when it received an HTTP request.

@kazu-yamamoto
Copy link
Author

Both hashable 1.2.0.4 and the current master (33a7234) do not solve this problem.

@bos bos reopened this Jan 10, 2013
@kazu-yamamoto
Copy link
Author

The following code caues this segfalt:

siphash-sse2.c: k0 = _mm_loadl_epi64((__m128i*)(&ik0));

@kazu-yamamoto
Copy link
Author

Since I could not found a way to make use of SSE2 on 32bit Linux, I just made a workaround for the 8byte alignment problem:

diff --git a/cbits/siphash.c b/cbits/siphash.c
index 34901b4..4b0c3d9 100644
--- a/cbits/siphash.c
+++ b/cbits/siphash.c
@@ -87,7 +87,7 @@ static inline u64 _siphash24(u64 k0, u64 k1, const u8 *str, si
     return _siphash(2, 4, k0, k1, str, len);
 }

-#if defined(__i386)
+#if defined(__i386) && (defined(_WIN64) || defined(__amd64__))
 # undef _siphash24

 static u64 (*_siphash24)(u64 k0, u64 k1, const u8 *, size_t);

Both on 64bit Linux and 32bit Linux, cabal tests are passed.

On 32bit Linux, (hash "/") results in -1811599090.
On 64bit Linux, it returns 8470997533827433892.
Is this correct behavior?

@kazu-yamamoto
Copy link
Author

Note that I have not tested this on Windows.

@bos
Copy link
Collaborator

bos commented Jan 10, 2013

@kazu-yamamoto I cannot reproduce your crash with the original test case on 32-bit Linux or Windows.

Also, it's expected that hashing gives different results on 32-bit and 64-bit systems, since Ints (and hence the default salt) are different sizes.

@kazu-yamamoto
Copy link
Author

I installed GHC 7.4.2 for this 32bit Linux today. It does not cause this problem. So, this problem is caused by combination of GHC 7.0.4 and 32bit Linux. (GHC 7.0.4 on 64bit Linux does not cause this problem.)

@bos bos closed this as completed in 9691f61 Jan 11, 2013
@bos
Copy link
Collaborator

bos commented Jan 11, 2013

Thanks, @kazu-yamamoto, that was the extra information I needed.

@kazu-yamamoto
Copy link
Author

My 32bit Linux (Linux 2.6.18) does not have le64toh in endian.h. Compiling hashable displays the following warning:

cbits/siphash.c:52:0:
     warning: implicit declaration of function ‘le64toh’

And GHC cannot compile my test program:

/home/kazu/.cabal/lib/hashable-1.2.0.4/ghc-7.0.4/libHShashable-1.2.0.4.a(siphash.o): In function _siphash_chunk.clone.0':
siphash.c:(.text+0x380): undefined reference to `le64toh'
/home/kazu/.cabal/lib/hashable-1.2.0.4/ghc-7.0.4/libHShashable-1.2.0.4.a(siphash.o): In function `hashable_siphash':
siphash.c:(.text+0x1050): undefined reference to `le64toh'
/home/kazu/.cabal/lib/hashable-1.2.0.4/ghc-7.0.4/libHShashable-1.2.0.4.a(siphash.o): In function `plain_siphash24':
siphash.c:(.text+0x1b40): undefined reference to `le64toh'
collect2: ld returned 1 exit status

@bos
Copy link
Collaborator

bos commented Jan 11, 2013

Your userland on that machine is 4 or more years old - endian.h was added to glibc in 2009. You're welcome to send a patch, but I'm not going to do anything about this, I'm afraid.

@kazu-yamamoto
Copy link
Author

FreeBSD does not have endian.h but has sys/endian.h including le64toh.
Mac does not have endian.h. And I cannot find le64toh anywhere.

To bury differences, may I introduce "configure"?

@kazu-yamamoto
Copy link
Author

I sent a pull request to solve this problem without using configure. What do you think?

kazu-yamamoto@556796c

@bos
Copy link
Collaborator

bos commented Jan 11, 2013

Very similar to 1360f47, which I was writing at the same time.

@kazu-yamamoto
Copy link
Author

I confirmed that the current master works well on:

  • MacOS (Mountain Lion)
  • 32bit Linux
  • 64bit Linux
  • FreeBSD

Thank you very much.

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

Successfully merging a pull request may close this issue.

2 participants