BUG: Fix dot and inner type/value exception failures #6988

jakirkham · 2016-01-09T23:32:28Z

Partial fix: #6990

TL;DR: Bug fix to catch a possible second exception while building an exception message. Includes tests for dot and inner where this bug was first discovered.

History: Initially, this PR began as tests for the current behavior of dot and inner when they have some mismatched types that cannot be coerced into some common working type. However, this resulted in discovering a special class of bugs in C where an exception could occur during the creation of the exception message, but was not being handled. This bug has been carefully described with a list of instances in this bug report ( #6990 ). As a result, this contains a fix for one instance of this bug and tests to verify that this instance is fixed.

jakirkham · 2016-01-10T00:12:35Z

Seeing failure on USE_DEBUG=1 only here, as well. Not sure what is going on. Maybe related to this ( http://bugs.python.org/issue20500 ). Though it is supposedly fixed. Maybe related to this ( #6741 ).

jakirkham · 2016-01-10T06:20:33Z

This definitely feels like an interpreter bug. I tried running this with python-dbg version 2.7.11 and did not encounter this error.

jakirkham · 2016-01-10T06:30:47Z

For clarity, this is what I am seeing...

python3-dbg: ../Objects/object.c:463: PyObject_Repr: Assertion `!PyErr_Occurred()' failed.
./tools/travis-test.sh: line 108:  5108 Aborted                 (core dumped) $PYTHON ../tools/test-installed-numpy.py

seberg · 2016-01-10T09:29:57Z

It sounds a bit like an error set without an error return (i.e. a C-function does PyErr_SetString, but then does not return NULL). The only thing I can think of is that it wants a PyErr_Clear() before setting the new error, though I don't remember that this was necessary.

jakirkham · 2016-01-10T17:30:30Z

What about returning -1? It seems there are some cases -1 is returned instead.

seberg · 2016-01-10T17:36:49Z

Same as NULL in that case, there are two calling conventions for python function, those functions just don't return a python object in general.

jakirkham · 2016-01-10T18:22:52Z

So, I have looked, but nothing obviously sticks out to me. The only thing I can think of is PyArray_DescrFromType returning NULL in some cases, but doesn't use PyErr_SetString in all cases. Here is the one time it is used. (

numpy/numpy/core/src/multiarray/arraytypes.c.src

Lines 4442 to 4443 in 5dab927

    
           PyErr_SetString(PyExc_ValueError, 
        
                   "Invalid data-type for array");

)

jakirkham · 2016-01-10T18:28:41Z

The same can be said for PyArray_FromAny.

jakirkham · 2016-01-10T19:09:57Z

I added PyErr_Clear above PyErr_SetString and it made no difference.

jakirkham · 2016-01-10T20:25:48Z

numpy/core/tests/test_multiarray.py

+        A = np.array((1,1), dtype='i,i')
+
+        assert_raises(ValueError, np.dot, c, A)
+        assert_raises(TypeError, np.dot, A, c)


So, I managed to get this built so I can debug with python3-dbg interactively and discovered this line will cause the strange exception. However, the line before does not cause the exception.

jakirkham · 2016-01-10T20:44:27Z

So, the error in the second case looks something like this.

TypeError: Cannot cast array data from dtype([('f0', '<i4'), ('f1', '<i4')]) to dtype('float64') according to the rule 'safe'

Based on what @charris has put forward, my guess is trying to create the representations of these dtypes to fill into the exception is causing the problem. Do either of you know where this is happening?

jakirkham · 2016-01-10T20:47:27Z

In short, things like this are the problem. Somehow the exception needs to be stored/cleared before doing this and then recreated afterwards. (

numpy/numpy/core/src/multiarray/array_assign_scalar.c

Line 207 in 97c3536

PyObject_Repr((PyObject *)src_dtype));

)

jakirkham · 2016-01-10T20:55:51Z

I have opened a new issue ( #6990 ) related to this problem.

jakirkham · 2016-01-10T22:37:21Z

So, I tried to add a bug fix, but I am seeing a segmentation fault. However, I don't see it the first time I test the dot or inner case of this form assert_raises(TypeError, <func>, A, c), but only the second time. Does anything look obviously wrong to either of you with this bug fix?

jakirkham · 2016-01-10T22:58:36Z

Tried running gdb and it points me to this line ( https://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/convert_datatype.c#L1010 ). Weird thing is I don't think we are calling that function at all.

jakirkham · 2016-01-10T23:32:15Z

numpy/core/src/multiarray/ctors.c

@@ -1926,14 +1926,35 @@ PyArray_FromArray(PyArrayObject *arr, PyArray_Descr *newtype, int flags)
    /* Raise an error if the casting rule isn't followed */
    if (!PyArray_CanCastArrayTo(arr, newtype, casting)) {


After some more debugging with gdb apparently the segmentation fault is occurring at this line. The real line responsible is in PyArray_EquivTypes. Somehow PyArray_EquivTypes is getting NULL for its first type, which is the cause of the problem.

jakirkham · 2016-01-11T00:05:15Z

So, this is kind of interesting. If I run the code below, I don't encounter the segmentation fault, but I do see the error message change after the first call. Namely, it changes to the error message raised by PyArray_MatrixProduct2 from the one that we have tried to fix here. The ValueError will continue to show up every time I run it. A similar thing happens with inner. However, if I try to reassign a value to A (not B) it will segment fault.

>>> import numpy
>>> import numpy as np
>>> A = np.array((1,1), dtype='i,i')
>>> B = np.array((1.,1.), dtype='f,f')
>>> np.dot(A, B)
TypeError: Cannot cast array data from dtype([('f0', '<i4'), ('f1', '<i4')]) to dtype('V8') according to the rule 'safe'
>>> np.dot(A, B)
ValueError: Cannot find a common data type.
...
>>> A = np.array((1,1), dtype='i,i')

jakirkham · 2016-01-11T00:15:54Z

numpy/core/src/multiarray/ctors.c

        errmsg = PyUString_FromString("Cannot cast array data from ");
-        PyUString_ConcatAndDel(&errmsg,
-                PyObject_Repr((PyObject *)PyArray_DESCR(arr)));
+        arr_descr = PyArray_DESCR(arr);


I think calling Py_DECREF(arr_descr); may have been the cause of the segmentation faults. At the time, I didn't realize PyArray_DESCR returned a borrowed reference. So, I was effectively deleting part of the array, but leaving the rest of the array around. As a result, we saw some weird behavior due to this.

jakirkham · 2016-01-11T00:20:06Z

I didn't realize that PyArray_DESCR shared a reference and was decrefing the returned object. I have fixed this so I am not doing this any more. Segmentation fault appears to be fixed locally. I think this will check out.

jakirkham · 2016-01-11T00:27:29Z

Alright, looks like it checks out. Please let me know if you guys see anything else here that needs changing.

jakirkham · 2016-01-11T00:56:28Z

numpy/core/src/multiarray/ctors.c

+            return NULL;
+        }
+        arr_descr_repr = PyObject_Repr((PyObject *)arr_descr);
+        Py_DECREF(arr_descr);


I think this may have been the cause of the segmentation faults. At the time, I didn't realize this returned a borrowed references. So, I was effectively deleting part of the array, but leaving the rest of the array around. As a result, we saw some weird behavior due to this.

jakirkham · 2016-01-11T14:36:12Z

Failures on AppVeyor ( #6991 ) had not previously occurred for this PR with exception of the segmentation fault issue (also happened on Travis), which has since been resolved. I believe these to be unrelated to the content of this PR.

jakirkham · 2016-01-11T15:46:51Z

As AppVeyor is merging with master, which is broken, it is currently failing the tests there. So, I ran my own AppVeyor build without this merge (this is rebased on a commit on master before the bad commit) and it looks like it checks out ( https://ci.appveyor.com/project/jakirkham/numpy/build/1.0.2 ).

njsmith · 2016-01-11T15:58:38Z

I'm starting to get really confused at what I'm looking at here -- the PR description says it's new tests, the discussion is about some gnarly C bug fixes... Maybe in the future it would work better to bundle up the test changes and code changes and big fixes into one combined PR so they stay together as a unit? Or maybe I just haven't been paying enough attention, that happens too sometimes :-)

jakirkham · 2016-01-11T16:05:46Z

It is pretty easy to get lost here. This is what happened.

Made this PR ( ENH, MAINT: Refactor PyArray_InnerProduct to use PyArray_MatrixProduct2 #6968 ).
Added simple tests for exception handling.
Tests got weird interpreter crash in debug mode.
Broke out the tests into current PR to see if they worked.
Surprisingly still got an interpreter crash in debug mode.
Worked with @seberg and @charris to understand weird interpreter crash in debug mode.
Figured out that this is due to one of many bugs fitting this class ( BUG: Clear errors before contructing error message that use PyObject_Repr #6990 ).
Added a bug fix here for this particular case.
Made this available in PR ( ENH, MAINT: Refactor PyArray_InnerProduct to use PyArray_MatrixProduct2 #6968 ) again.

Sorry this is super confusing. It was never my intent to confuse anyone like this. The bug discovered was really surprising and quite subtle. Hope this clarifies what happened. If you have more questions, please ask.

jakirkham · 2016-01-11T16:17:15Z

Updated the PR title and description to better explain what is going on here now.

@njsmith, is this making more sense yet? If not please just ask, this bug was surprisingly tricky to find. The fix ends up being straightforward.

jakirkham · 2016-01-11T16:18:03Z

Maybe in the future it would work better to bundle up the test changes and code changes and big fixes into one combined PR so they stay together as a unit? Or maybe I just haven't been paying enough attention, that happens too sometimes :-)

They are. It's ok. :)

…to a common type.

…into a common type.

…Object_Repr`. Also, do a better job of handling any errors raised while constructing the error message.

jakirkham · 2016-01-12T19:12:02Z

Thanks everyone for your help. In particular, thanks to @seberg and @charris for helping me locate and fix this bug.

This was referenced Jan 9, 2016

ENH, MAINT: Refactor PyArray_InnerProduct to use PyArray_MatrixProduct2 #6968

Merged

TST, MAINT: Make sure exceptions of inner and dot match for different cases #6987

Merged

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch 2 times, most recently from 0c77ec5 to de1e234 Compare January 10, 2016 01:05

charris added 05 - Testing component: numpy._core labels Jan 10, 2016

jakirkham reviewed Jan 10, 2016
View reviewed changes

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 9cfffe6 to 0ef99e0 Compare January 10, 2016 22:13

jakirkham changed the title ~~TST: Test dot and inner type/value failures~~ BUG, TST: Test dot and inner type/value failures Jan 10, 2016

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch 2 times, most recently from f41fdaf to 21e1e27 Compare January 10, 2016 22:23

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 21e1e27 to 860474b Compare January 10, 2016 22:40

jakirkham reviewed Jan 10, 2016
View reviewed changes

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 860474b to 2835d7f Compare January 11, 2016 00:13

jakirkham reviewed Jan 11, 2016
View reviewed changes

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 2835d7f to 860474b Compare January 11, 2016 00:55

jakirkham reviewed Jan 11, 2016
View reviewed changes

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 860474b to 2835d7f Compare January 11, 2016 00:56

jakirkham mentioned this pull request Jan 11, 2016

BUG: Clear errors before contructing error message that use PyObject_Repr #6990

Closed

16 tasks

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 2835d7f to b704320 Compare January 11, 2016 05:30

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from b704320 to 031028f Compare January 11, 2016 14:40

jakirkham changed the title ~~BUG, TST: Test dot and inner type/value failures~~ BUG: Test dot and inner type/value failures Jan 11, 2016

jakirkham changed the title ~~BUG: Test dot and inner type/value failures~~ BUG: Fix dot and inner type/value exception failures Jan 11, 2016

jakirkham added 3 commits January 11, 2016 19:42

TST: Ensure dot fails correctly if array types cannot be coerced in…

b491cc8

…to a common type.

TST: Ensure inner fails correctly if array types cannot be coerced …

67592d3

…into a common type.

BUG: Clear error before constructing error message using calls to `Py…

bab118d

…Object_Repr`. Also, do a better job of handling any errors raised while constructing the error message.

jakirkham force-pushed the test_dot_inner_type_failures_value_failure branch from 031028f to bab118d Compare January 12, 2016 00:42

charris merged commit bab118d into numpy:master Jan 12, 2016

jakirkham deleted the test_dot_inner_type_failures_value_failure branch January 12, 2016 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix dot and inner type/value exception failures #6988

BUG: Fix dot and inner type/value exception failures #6988

jakirkham commented Jan 9, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

seberg commented Jan 10, 2016

jakirkham commented Jan 10, 2016

seberg commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham Jan 10, 2016

jakirkham commented Jan 11, 2016

jakirkham Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

njsmith commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 12, 2016

		@@ -1926,14 +1926,35 @@ PyArray_FromArray(PyArrayObject arr, PyArray_Descr newtype, int flags)
		/* Raise an error if the casting rule isn't followed */
		if (!PyArray_CanCastArrayTo(arr, newtype, casting)) {

BUG: Fix dot and inner type/value exception failures #6988

BUG: Fix dot and inner type/value exception failures #6988

Conversation

jakirkham commented Jan 9, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

seberg commented Jan 10, 2016

jakirkham commented Jan 10, 2016

seberg commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham Jan 10, 2016

Choose a reason for hiding this comment

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham commented Jan 10, 2016

jakirkham Jan 10, 2016

Choose a reason for hiding this comment

jakirkham commented Jan 11, 2016

jakirkham Jan 11, 2016

Choose a reason for hiding this comment

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham Jan 11, 2016

Choose a reason for hiding this comment

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

njsmith commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 11, 2016

jakirkham commented Jan 12, 2016