[master] Merge branch 'trac1883'

src/lib/dns/python/name_python.cc

@@ -522,8 +522,9 @@ Name_isWildCard(s_Name* self) {
 
 Py_hash_t
 Name_hash(PyObject* pyself) {
-    s_Name* const self = static_cast<s_Name*>(pyself);
-    return (LabelSequence(*self->cppobj).getHash(false));
+    const s_Name* const self = static_cast<s_Name*>(pyself);
+    return (convertToPyHash<size_t>(
+                LabelSequence(*self->cppobj).getHash(false)));
 }
 
 } // end of unnamed namespace

src/lib/dns/python/pydnspp_common.h

@@ -17,6 +17,9 @@
 
 #include <Python.h>
 
+#include <boost/static_assert.hpp>
+
+#include <climits>  // for CHAR_BIT
 #include <stdexcept>
 #include <string>
 
@@ -48,6 +51,58 @@ int addClassVariable(PyTypeObject& c, const char* name, PyObject* obj);
 #if PY_MINOR_VERSION < 2
 typedef long Py_hash_t;
 #endif
+
+/// \brief Convert a hash value of arbitrary type to a Python hash value.
+///
+/// This templated function is a convenient wrapper to produce a valid hash
+/// value of type Py_hash_t, which is expected to be used as the return value
+/// of a PyTypeObject.tp_hash implementation.  Py_hash_t is a signed integer
+/// the same size as Py_ssize_t.
+///
+/// The major concern is that -1 means an error in tp_hash and so we need to
+/// ensure that this value is never returned.
+///
+/// If the size of the original hash type is small enough, we convert
+/// the original hash value to a value of Py_hash_t, resetting all higher bits
+/// to 0.  Due to the assumption on the sizes, the conversion to HashvalType
+/// is safe, and (after clearing the higher bits) results in a valid positive
+/// value.
+///
+/// If the size of the input and return types is the same, we clear the
+/// highest bit of the original hash so the resulting value will be in a valid
+/// positive range of Py_hash_t.  If the original type is unsigned, there's
+/// probably no better conversion than this because otherwise the conversion
+/// to Py_hash_t could result in an undefined behavior.  If the original type
+/// is signed, this conversion reduces the effective value range of the
+/// original hash.  If this is not desired, the caller should convert it
+/// by itself (note that -1 should be still avoided).
+///
+/// This function does not support the case where the size of the original
+/// hash type is larger than that of Py_hash_t.
+template <typename HashvalType>
+Py_hash_t
+convertToPyHash(HashvalType val) {
+    BOOST_STATIC_ASSERT(sizeof(HashvalType) <= sizeof(Py_hash_t));
+
+    // Some versions of g++ doesn't ignore the impossible case of if/else
+    // below (depending on the size of HashvalType) and triggers a false
+    // warning.
+    // To work around it we use an intermediate mutable variable.
+    // See Trac #1883 for details.
+    size_t hash_val_bits = CHAR_BIT * sizeof(HashvalType);
+
+    if (sizeof(HashvalType) < sizeof(Py_hash_t)) {
+        // The original hash type is small enough.  Do trivial conversion.
+        const Py_hash_t mask = ~(static_cast<Py_hash_t>(-1) << hash_val_bits);
+        return (static_cast<Py_hash_t>(val) & mask);
+    } else {
+        // Clear the highest bit of the original hash so the conversion is
+        // safe and avoids -1.
+        HashvalType mask = ~(static_cast<HashvalType>(1) <<
+                             (hash_val_bits - 1));
+        return (val & mask);
+    }
+}
 } // namespace python
 } // namespace dns
 } // namespace isc

src/lib/dns/python/rrclass_python.cc

@@ -267,8 +267,8 @@ PyObject* RRClass_ANY(s_RRClass*) {
 
 Py_hash_t
 RRClass_hash(PyObject* pyself) {
-    s_RRClass* const self = static_cast<s_RRClass*>(pyself);
-    return (self->cppobj->getCode());
+    const s_RRClass* const self = static_cast<s_RRClass*>(pyself);
+    return (convertToPyHash<uint16_t>(self->cppobj->getCode()));
 }
 
 } // end anonymous namespace

src/lib/dns/python/rrtype_python.cc

@@ -49,6 +49,7 @@ PyObject* RRType_str(PyObject* self);
 PyObject* RRType_toWire(s_RRType* self, PyObject* args);
 PyObject* RRType_getCode(s_RRType* self);
 PyObject* RRType_richcmp(s_RRType* self, s_RRType* other, int op);
+Py_hash_t RRType_hash(PyObject* pyself);
 PyObject* RRType_NSEC3PARAM(s_RRType *self);
 PyObject* RRType_DNAME(s_RRType *self);
 PyObject* RRType_PTR(s_RRType *self);
@@ -368,6 +369,11 @@ RRType_ANY(s_RRType*) {
     return (RRType_createStatic(RRType::ANY()));
 }
 
+Py_hash_t
+RRType_hash(PyObject* pyself) {
+    const s_RRType* const self = static_cast<s_RRType*>(pyself);
+    return (convertToPyHash<uint16_t>(self->cppobj->getCode()));
+}
 } // end anonymous namespace
 
 namespace isc {
@@ -394,7 +400,7 @@ PyTypeObject rrtype_type = {
     NULL,                               // tp_as_number
     NULL,                               // tp_as_sequence
     NULL,                               // tp_as_mapping
-    NULL,                               // tp_hash
+    RRType_hash,                        // tp_hash
     NULL,                               // tp_call
     RRType_str,                         // tp_str
     NULL,                               // tp_getattro

src/lib/dns/python/tests/rrtype_python_test.py

@@ -116,6 +116,15 @@ class TestModuleSpec(unittest.TestCase):
 
         self.assertFalse(self.rrtype_1 == 1)
 
+    def test_hash(self):
+        # Exploiting the knowledge that the hash value is the numeric class
+        # value, we can predict the comparison result.
+        self.assertEqual(hash(RRType.AAAA()), hash(RRType("AAAA")))
+        self.assertEqual(hash(RRType("aaaa")), hash(RRType("AAAA")))
+        self.assertEqual(hash(RRType(28)), hash(RRType("AAAA")))
+        self.assertNotEqual(hash(RRType.A()), hash(RRType.NS()))
+        self.assertNotEqual(hash(RRType.AAAA()), hash(RRType("Type65535")))
+
     def test_statics(self):
         self.assertEqual(RRType("NSEC3PARAM"), RRType.NSEC3PARAM())
         self.assertEqual(RRType("DNAME"), RRType.DNAME())