Merge lp:~jameinel/bzr/2.1-pyrex-64bit into lp:bzr

Proposed by John A Meinel
Status: Merged
Approved by: Ian Clatworthy
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.1-pyrex-64bit
Merge into: lp:bzr
Diff against target: 55 lines
1 file modified
bzrlib/_simple_set_pyx.pyx (+9/-11)
To merge this branch: bzr merge lp:~jameinel/bzr/2.1-pyrex-64bit
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Review via email: mp+13292@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Vincent's wonderful buildbot farm caught a regression with the new code.

Namely, pyrex 0.9.8.5 treats 'hash(obj)' as though it returns an int, rather than a long. So on 64-bit machines it truncates the hash value. This was an issue because sometimes we accessed the hash directly. (Py_TYPE(obj).tp_hash(obj)).

The attached patch just changes all lookups to go through PyObject_Hash() which is defined to give a stable interface. It also means we don't have to manually do exception checking, because the return of -1 can be defined as an exception condition.

Note that pyrex 0.9.7.2 was doing the right thing here, which is why Vincent's 64-bit Ubuntu machines were not failing, but the 64-bit Freebsd machines were. (freebsd had a newer pyrex...)

Revision history for this message
Robert Collins (lifeless) wrote :

 +1

is there a bug filed on upstrea pyrex about its handling of hash()?

-Rob

Revision history for this message
Andrew Bennetts (spiv) wrote :

> +1

Apparently LP doesn't consider that a proper vote :/

> is there a bug filed on upstrea pyrex about its handling of hash()?

Yes (well, I saw John post to the mailing list, which is the closest thing Pyrex has to a bug tracker AFAIK).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/_simple_set_pyx.pyx'
--- bzrlib/_simple_set_pyx.pyx 2009-10-12 16:48:36 +0000
+++ bzrlib/_simple_set_pyx.pyx 2009-10-13 16:50:20 +0000
@@ -37,6 +37,9 @@
3737
38 PyTypeObject *Py_TYPE(PyObject *)38 PyTypeObject *Py_TYPE(PyObject *)
39 int PyObject_IsTrue(PyObject *)39 int PyObject_IsTrue(PyObject *)
40 # Note: *Don't* use hash(), Pyrex 0.9.8.5 thinks it returns an 'int', and
41 # thus silently truncates to 32-bits on 64-bit machines.
42 long PyObject_Hash(PyObject *) except -1
40 43
41 void *PyMem_Malloc(size_t nbytes)44 void *PyMem_Malloc(size_t nbytes)
42 void PyMem_Free(void *)45 void PyMem_Free(void *)
@@ -61,11 +64,7 @@
6164
62 if this == other:65 if this == other:
63 return 166 return 1
64 other_hash = Py_TYPE(other).tp_hash(other)67 other_hash = PyObject_Hash(other)
65 if other_hash == -1:
66 # Even though other successfully hashed in the past, it seems to have
67 # changed its mind, and failed this time, so propogate the failure.
68 return -1
69 if other_hash != this_hash:68 if other_hash != this_hash:
70 return 069 return 0
7170
@@ -203,9 +202,7 @@
203 mask = self._mask202 mask = self._mask
204 table = self._table203 table = self._table
205204
206 the_hash = Py_TYPE(key).tp_hash(key)205 the_hash = PyObject_Hash(key)
207 if the_hash == -1:
208 return -1
209 i = the_hash206 i = the_hash
210 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever207 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever
211 slot = &table[i & mask]208 slot = &table[i & mask]
@@ -458,13 +455,14 @@
458 cdef long key_hash455 cdef long key_hash
459 cdef PyObject **table, **slot, *cur, **free_slot, *py_key456 cdef PyObject **table, **slot, *cur, **free_slot, *py_key
460457
461 # hash is a signed long(), we are using an offset at unsigned size_t458 py_key = <PyObject *>key
462 key_hash = hash(key)459 # Note: avoid using hash(obj) because of a bug w/ pyrex 0.9.8.5 and 64-bit
460 # (it treats hash() as returning an 'int' rather than a 'long')
461 key_hash = PyObject_Hash(py_key)
463 i = <size_t>key_hash462 i = <size_t>key_hash
464 mask = self._mask463 mask = self._mask
465 table = self._table464 table = self._table
466 free_slot = NULL465 free_slot = NULL
467 py_key = <PyObject *>key
468 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever466 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever
469 slot = &table[i & mask]467 slot = &table[i & mask]
470 cur = slot[0]468 cur = slot[0]