Bazaar

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

  1. 2.1-pyrex-64bit
  2. Merge into bzr.dev
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
1=== modified file 'bzrlib/_simple_set_pyx.pyx'
2--- bzrlib/_simple_set_pyx.pyx 2009-10-12 16:48:36 +0000
3+++ bzrlib/_simple_set_pyx.pyx 2009-10-13 16:50:20 +0000
4@@ -37,6 +37,9 @@
5
6 PyTypeObject *Py_TYPE(PyObject *)
7 int PyObject_IsTrue(PyObject *)
8+ # Note: *Don't* use hash(), Pyrex 0.9.8.5 thinks it returns an 'int', and
9+ # thus silently truncates to 32-bits on 64-bit machines.
10+ long PyObject_Hash(PyObject *) except -1
11
12 void *PyMem_Malloc(size_t nbytes)
13 void PyMem_Free(void *)
14@@ -61,11 +64,7 @@
15
16 if this == other:
17 return 1
18- other_hash = Py_TYPE(other).tp_hash(other)
19- if other_hash == -1:
20- # Even though other successfully hashed in the past, it seems to have
21- # changed its mind, and failed this time, so propogate the failure.
22- return -1
23+ other_hash = PyObject_Hash(other)
24 if other_hash != this_hash:
25 return 0
26
27@@ -203,9 +202,7 @@
28 mask = self._mask
29 table = self._table
30
31- the_hash = Py_TYPE(key).tp_hash(key)
32- if the_hash == -1:
33- return -1
34+ the_hash = PyObject_Hash(key)
35 i = the_hash
36 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever
37 slot = &table[i & mask]
38@@ -458,13 +455,14 @@
39 cdef long key_hash
40 cdef PyObject **table, **slot, *cur, **free_slot, *py_key
41
42- # hash is a signed long(), we are using an offset at unsigned size_t
43- key_hash = hash(key)
44+ py_key = <PyObject *>key
45+ # Note: avoid using hash(obj) because of a bug w/ pyrex 0.9.8.5 and 64-bit
46+ # (it treats hash() as returning an 'int' rather than a 'long')
47+ key_hash = PyObject_Hash(py_key)
48 i = <size_t>key_hash
49 mask = self._mask
50 table = self._table
51 free_slot = NULL
52- py_key = <PyObject *>key
53 for n_lookup from 0 <= n_lookup <= <size_t>mask: # Don't loop forever
54 slot = &table[i & mask]
55 cur = slot[0]