Merge lp:~jameinel/bzr/2.0.4-pyrex-propagation into lp:bzr/2.0

[John A Meinel]

[John A Meinel (jameinel)]

This addresses Andrew's follow up about my earlier fix.

It adds a 'test_source' test that all 'cdef' functions have thought about how they want to handle exceptions. Either they

1) Return an object
2) Return a class defined in local scope (which is an object)
3) Have an 'except ?' clause
4) Add "# cannot_raise" (useful for pure C functions that know they cannot raise)

I ran this over the .pyx files and fixed them all to conform. It does catch
+ cdef int _gather_result_for_consistency(self, result) except -1:

and _knit_load_data's
- cdef void validate(self):
+ cdef int validate(self) except -1:

Otherwise, all other cases are 'cannot_raise'. I expect to find a few more in the 2.1 branch, which will require more work than just merging 2.0 => bzr.dev once this lands. I'll create a branch for it soon.

[Andrew Bennetts (spiv)]

This looks good to me. I worry a little bit that the # cannot_raise annotations might become stale over time, but this seems to be as good as we can do without improving Pyrex itself.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> This looks good to me. I worry a little bit that the # cannot_raise annotations might become stale over time, but this seems to be as good as we can do without improving Pyrex itself.

So I have a block that checks if you have both a "cannot_raise" and a
"except :" clause.

But certainly it is possible that *today* I add a 'cannot_raise' and
then in the future someone adds a line that calls into python code, and
silently traps the exception. I think, as you say, the only way forward
is to improve Pyrex/Cython. I should try asking Cython what they think
it would take to implement this.

cannot_raise such that it can only call other cannot_raise functions,
and no pure-python functions. They already have 'nogil' which does some
of that. I wonder if we could cheat and use 'nogil' to get that
functionality.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktElrAACgkQJdeBCYSNAAOW5QCffahehGhbJeqh8Vz5U9nNBpME
K/gAoNXM/lGOhy+aBj86tRFscLZeUn1S
=+VOe
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2010-01-05 00:01:54 +0000
+++ NEWS	2010-01-06 17:31:11 +0000
@@ -61,6 +61,11 @@
 Testing
 *******
 
+* We have a new ``test_source`` that ensures all pyrex ``cdef`` functions
+  handle exceptions somehow. (Possibly by setting ``# cannot_raise``
+  rather than an ``except ?:`` clause.) This should help prevent bugs like
+  bug #495023. (John Arbash Meinel)
+
 
 bzr 2.0.3
 #########

=== modified file 'bzrlib/_annotator_pyx.pyx'
--- bzrlib/_annotator_pyx.pyx	2009-07-08 17:09:03 +0000
+++ bzrlib/_annotator_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -83,7 +83,14 @@
     return 0
 
 
-cdef PyObject *_next_tuple_entry(object tpl, Py_ssize_t *pos):
+cdef PyObject *_next_tuple_entry(object tpl, Py_ssize_t *pos): # cannot_raise
+    """Return the next entry from this tuple.
+
+    :param tpl: The tuple we are investigating, *must* be a PyTuple
+    :param pos: The last item we found. Will be updated to the new position.
+    
+    This cannot raise an exception, as it does no error checking.
+    """
     pos[0] = pos[0] + 1
     if pos[0] >= PyTuple_GET_SIZE(tpl):
         return NULL

=== modified file 'bzrlib/_btree_serializer_pyx.pyx'
--- bzrlib/_btree_serializer_pyx.pyx	2009-10-01 20:50:36 +0000
+++ bzrlib/_btree_serializer_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -54,7 +54,7 @@
 
 
 # TODO: Find some way to import this from _dirstate_helpers
-cdef void* _my_memrchr(void *s, int c, size_t n):
+cdef void* _my_memrchr(void *s, int c, size_t n): # cannot_raise
     # memrchr seems to be a GNU extension, so we have to implement it ourselves
     # It is not present in any win32 standard library
     cdef char *pos

=== modified file 'bzrlib/_chk_map_pyx.pyx'
--- bzrlib/_chk_map_pyx.pyx	2009-06-22 12:52:39 +0000
+++ bzrlib/_chk_map_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -65,7 +65,7 @@
 _unknown = None
 
 # We shouldn't just copy this from _dirstate_helpers_pyx
-cdef void* _my_memrchr(void *s, int c, size_t n):
+cdef void* _my_memrchr(void *s, int c, size_t n): # cannot_raise
     # memrchr seems to be a GNU extension, so we have to implement it ourselves
     cdef char *pos
     cdef char *start

=== modified file 'bzrlib/_dirstate_helpers_pyx.pyx'
--- bzrlib/_dirstate_helpers_pyx.pyx	2010-01-04 20:06:30 +0000
+++ bzrlib/_dirstate_helpers_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -119,7 +119,7 @@
     # void *memrchr(void *s, int c, size_t len)
 
 
-cdef void* _my_memrchr(void *s, int c, size_t n):
+cdef void* _my_memrchr(void *s, int c, size_t n): # cannot_raise
     # memrchr seems to be a GNU extension, so we have to implement it ourselves
     cdef char *pos
     cdef char *start
@@ -165,7 +165,7 @@
     return PyString_FromStringAndSize(s, size)
 
 
-cdef int _is_aligned(void *ptr):
+cdef int _is_aligned(void *ptr): # cannot_raise
     """Is this pointer aligned to an integer size offset?
 
     :return: 1 if this pointer is aligned, 0 otherwise.
@@ -173,7 +173,7 @@
     return ((<intptr_t>ptr) & ((sizeof(int))-1)) == 0
 
 
-cdef int _cmp_by_dirs(char *path1, int size1, char *path2, int size2):
+cdef int _cmp_by_dirs(char *path1, int size1, char *path2, int size2): # cannot_raise
     cdef unsigned char *cur1
     cdef unsigned char *cur2
     cdef unsigned char *end1
@@ -295,7 +295,7 @@
 
 
 cdef int _cmp_path_by_dirblock_intern(char *path1, int path1_len,
-                                      char *path2, int path2_len):
+                                      char *path2, int path2_len): # cannot_raise
     """Compare two paths by what directory they are in.
 
     see ``_cmp_path_by_dirblock`` for details.
@@ -768,7 +768,7 @@
     state._dirblock_state = DirState.IN_MEMORY_UNMODIFIED
 
 
-cdef int minikind_from_mode(int mode):
+cdef int minikind_from_mode(int mode): # cannot_raise
     # in order of frequency:
     if S_ISREG(mode):
         return c"f"
@@ -915,7 +915,8 @@
     return link_or_sha1
 
 
-cdef char _minikind_from_string(object string):
+# TODO: Do we want to worry about exceptions here?
+cdef char _minikind_from_string(object string) except? -1:
     """Convert a python string to a char."""
     return PyString_AsString(string)[0]
 
@@ -953,7 +954,7 @@
     raise KeyError(PyString_FromStringAndSize(_minikind, 1))
 
 
-cdef int _versioned_minikind(char minikind):
+cdef int _versioned_minikind(char minikind): # cannot_raise
     """Return non-zero if minikind is in fltd"""
     return (minikind == c'f' or
             minikind == c'd' or
@@ -1373,7 +1374,7 @@
     def iter_changes(self):
         return self
 
-    cdef void _gather_result_for_consistency(self, result):
+    cdef int _gather_result_for_consistency(self, result) except -1:
         """Check a result we will yield to make sure we are consistent later.
         
         This gathers result's parents into a set to output later.
@@ -1381,7 +1382,7 @@
         :param result: A result tuple.
         """
         if not self.partial or not result[0]:
-            return
+            return 0
         self.seen_ids.add(result[0])
         new_path = result[1][1]
         if new_path:
@@ -1391,6 +1392,7 @@
             # Add the root directory which parent_directories does not
             # provide.
             self.search_specific_file_parents.add('')
+        return 0
 
     cdef int _update_current_block(self) except -1:
         if (self.block_index < len(self.state._dirblocks) and

=== modified file 'bzrlib/_groupcompress_pyx.pyx'
--- bzrlib/_groupcompress_pyx.pyx	2009-06-10 03:56:49 +0000
+++ bzrlib/_groupcompress_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -276,7 +276,7 @@
 
 
 cdef unsigned char *_decode_copy_instruction(unsigned char *bytes,
-    unsigned char cmd, unsigned int *offset, unsigned int *length):
+    unsigned char cmd, unsigned int *offset, unsigned int *length): # cannot_raise
     """Decode a copy instruction from the next few bytes.
 
     A copy instruction is a variable number of bytes, so we will parse the

=== modified file 'bzrlib/_knit_load_data_pyx.pyx'
--- bzrlib/_knit_load_data_pyx.pyx	2009-06-22 12:52:39 +0000
+++ bzrlib/_knit_load_data_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -97,11 +97,12 @@
         self.end_str = NULL
         self.history_len = 0
 
-    cdef void validate(self):
+    cdef int validate(self) except -1:
         if not PyDict_CheckExact(self.cache):
             raise TypeError('kndx._cache must be a python dict')
         if not PyList_CheckExact(self.history):
             raise TypeError('kndx._history must be a python list')
+        return 0
 
     cdef object process_options(self, char *option_str, char *end):
         """Process the options string into a list."""

=== modified file 'bzrlib/_known_graph_pyx.pyx'
--- bzrlib/_known_graph_pyx.pyx	2009-09-02 13:32:52 +0000
+++ bzrlib/_known_graph_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -591,7 +591,7 @@
             self._revno_first, self._revno_second, self._revno_last,
             self.is_first_child, self.seen_by_child)
 
-    cdef int has_pending_parents(self):
+    cdef int has_pending_parents(self): # cannot_raise
         if self.left_pending_parent is not None or self.pending_parents:
             return 1
         return 0

=== modified file 'bzrlib/_rio_pyx.pyx'
--- bzrlib/_rio_pyx.pyx	2009-05-15 02:36:03 +0000
+++ bzrlib/_rio_pyx.pyx	2010-01-06 17:31:11 +0000
@@ -49,7 +49,7 @@
 
 from bzrlib.rio import Stanza
 
-cdef int _valid_tag_char(char c):
+cdef int _valid_tag_char(char c): # cannot_raise
     return (c == c'_' or c == c'-' or 
             (c >= c'a' and c <= c'z') or
             (c >= c'A' and c <= c'Z') or

=== modified file 'bzrlib/_walkdirs_win32.pyx'
--- bzrlib/_walkdirs_win32.pyx	2009-03-23 14:59:43 +0000
+++ bzrlib/_walkdirs_win32.pyx	2010-01-06 17:31:11 +0000
@@ -109,7 +109,7 @@
                                  wcslen(data.cFileName))
 
 
-cdef int _get_mode_bits(WIN32_FIND_DATAW *data):
+cdef int _get_mode_bits(WIN32_FIND_DATAW *data): # cannot_raise
     cdef int mode_bits
 
     mode_bits = 0100666 # writeable file, the most common
@@ -121,13 +121,13 @@
     return mode_bits
 
 
-cdef __int64 _get_size(WIN32_FIND_DATAW *data):
+cdef __int64 _get_size(WIN32_FIND_DATAW *data): # cannot_raise
     # Pyrex casts a DWORD into a PyLong anyway, so it is safe to do << 32
     # on a DWORD
     return ((<__int64>data.nFileSizeHigh) << 32) + data.nFileSizeLow
 
 
-cdef double _ftime_to_timestamp(FILETIME *ft):
+cdef double _ftime_to_timestamp(FILETIME *ft): # cannot_raise
     """Convert from a FILETIME struct into a floating point timestamp.
 
     The fields of a FILETIME structure are the hi and lo part
@@ -147,7 +147,7 @@
     return (val * 1.0e-7) - 11644473600.0
 
 
-cdef int _should_skip(WIN32_FIND_DATAW *data):
+cdef int _should_skip(WIN32_FIND_DATAW *data): # cannot_raise
     """Is this '.' or '..' so we should skip it?"""
     if (data.cFileName[0] != c'.'):
         return 0

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2009-10-26 22:03:28 +0000
+++ bzrlib/tests/test_selftest.py	2010-01-06 17:31:11 +0000
@@ -892,6 +892,8 @@
         result.report_unsupported(test, feature)
         output = result_stream.getvalue()[prefix:]
         lines = output.splitlines()
+        # XXX: This is a timing dependent test. I've had it fail because it
+        #      took 6ms to evaluate... :(
         self.assertEqual(lines, ['NODEP        0ms',
                                  "    The feature 'Feature' is not available."])
 

=== modified file 'bzrlib/tests/test_source.py'
--- bzrlib/tests/test_source.py	2009-09-10 03:44:53 +0000
+++ bzrlib/tests/test_source.py	2010-01-06 17:31:11 +0000
@@ -369,3 +369,48 @@
             self.fail(
                 "these files contain an assert statement and should not:\n%s"
                 % '\n'.join(badfiles))
+
+    def test_extension_exceptions(self):
+        """Extension functions should propagate exceptions.
+
+        Either they should return an object, have an 'except' clause, or have a
+        "# cannot_raise" to indicate that we've audited them and defined them as not
+        raising exceptions.
+        """
+        both_exc_and_no_exc = []
+        missing_except = []
+        class_re = re.compile(r'^(cdef\s+)?class (\w+).*:', re.MULTILINE)
+        except_re = re.compile(r'cdef\s*' # start with cdef
+                               r'([\w *]*?)\s*' # this is the return signature
+                               r'(\w+)\s*\(' # the function name
+                               r'[^)]*\)\s*' # parameters
+                               r'(.*)\s*:' # the except clause
+                               r'\s*(#\s*cannot[- _]raise)?' # cannot raise comment
+                              )
+        for fname, text in self.get_source_file_contents(
+                extensions=('.pyx',)):
+            known_classes = set([m[1] for m in class_re.findall(text)])
+            cdefs = except_re.findall(text)
+            for sig, func, exc_clause, no_exc_comment in cdefs:
+                if not sig or sig in known_classes:
+                    sig = 'object'
+                if exc_clause and no_exc_comment:
+                    both_exc_and_no_exc.append((fname, func))
+                if sig != 'object' and not (exc_clause or no_exc_comment):
+                    missing_except.append((fname, func))
+        error_msg = []
+        if both_exc_and_no_exc:
+            error_msg.append('The following functions had "cannot raise" comments'
+                             ' but did have an except clause set:')
+            for fname, func in both_exc_and_no_exc:
+                error_msg.append('%s:%s' % (fname, func))
+            error_msg.extend(('', ''))
+        if missing_except:
+            error_msg.append('The following functions have fixed return types,'
+                             ' but no except clause.')
+            error_msg.append('Either add an except or append "# cannot_raise".')
+            for fname, func in missing_except:
+                error_msg.append('%s:%s' % (fname, func))
+            error_msg.extend(('', ''))
+        if error_msg:
+            self.fail('\n'.join(error_msg))