Crash due to missing Cython exception handling for relational_to_bool()

[orlitzky]

sage: f(x)=matrix()                           
sage: bool(f(x)==f(x))

terminate called after throwing an instance of 'std::runtime_error'
  what():  Number_T::hash() python function (__hash__) raised exception
------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Sage will now terminate.
------------------------------------------------------------------------

Component: symbolics

Issue created by migration from https://trac.sagemath.org/ticket/14211

[rwst]

Upstream: Reported upstream. Developers acknowledge bug.

[rwst]

comment:7

Reported as pynac/pynac#34

[rwst]

comment:8

Minimal case given. In Sage hash(matrix()) will not allow hashing probably because PyObject_Hash will fail and to prevent that. The fail happens in Pynac nevertheless because the check is avoided due to function expansion and there leads to the crash.

One part of the fix is preventing Sage from crashing. Surrounding the call to Pynac in Expression::__nonzero__ with guards:

            sig_on()
            pynac_result = relational_to_bool(self._gobj)
            sig_off()

leads to:

sage: bool(f(x)==f(x))
terminate called after throwing an instance of 'std::runtime_error'
  what():  Number_T::hash() python function (__hash__) raised exception
---------------------------------------------------------------------------
TypeError: mutable matrices are unhashable

which is the same you get with hash(matrix()), so quite sensible. What more? Why are matrices generated by vector*vector (the original case) or those in a function definition (like above) mutable at all?

[rwst]

Changed upstream from Reported upstream. Developers acknowledge bug. to Reported upstream. Developers deny it's a bug.

[nbruin]

comment:11

Replying to @rwst:

which is the same you get with hash(matrix()), so quite sensible. What more? Why are matrices generated by vector*vector (the original case) or those in a function definition (like above) mutable at all?

Because the default for matrix creation is to create a mutable matrix. That's because a mutable matrix can be turned into an immutable one, but not the other way around (that requires copying the matrix to a new, mutable, one). That default is probably inconvenient for symbolics.

Are you sure this is the sole problem, though? If we try this:

def imm_mt(*args,**kwargs):
    M=matrix(*args,**kwargs)
    M.set_immutable()
    return M
f(x)=imm_mt()

then we get

sage: hash(f(x))
0

but bool(f(x)==f(x)) still crashes. is it trying to see if f(x)*f(x)^(-1) is one?

[rwst]

comment:12

Replying to @nbruin:

Are you sure this is the sole problem, though?

I have reopened the Pynac issue. The crash is in exactly the same call tree. It happens while trying to multiply the rhs with -1 to get lhs-rhs. So:

sage: f(x)=matrix()
sage: bool(f(x)==1)
True
sage: bool(1==f(x))
<crash>

Also no, I don't think it's the sole problem. Either Sage or Pynac will have to check somewhere before evaluating that all mutable Python objects of the expression are switched to immutable (or make immutable copies).

EDIT: But that would be a different ticket: #18360

[jdemeyer]

comment:13

Please read http://docs.cython.org/src/userguide/wrapping_CPlusPlus.html#exceptions

[rwst]

Commit: b9698e0

[rwst]

comment:17

Replying to @nbruin:

If we try this:

def imm_mt(*args,**kwargs):
    M=matrix(*args,**kwargs)
    M.set_immutable()
    return M
f(x)=imm_mt()

then we get

sage: hash(f(x))
0

but bool(f(x)==f(x)) still crashes.

My best guess at the moment is that somewhere in between a copy is made which, as you suggested, is then mutable again. This can only be fixed by a dedicated ticket such as #18360.

As to this code, the reviewer should check doubly if what I did here was right. There was not a single example in Sage with a special function catching exceptions from which I could have learned.

---

New commits:

b9698e0

14211: catch exceptions from Expression.__nonzero__

[jdemeyer]

comment:18

Obviously, a doctest is needed.

Besides that, it makes no sense to add an additional layer _nonzero_. The exception should be handled in the pynac interface, i.e. the relational_to_bool() function.

[rwst]

Changed branch from u/rws/crash_due_to_missing_cython_exception_handling_for_relational_to_bool__ to u/rws/14211

[rwst]

comment:20

Interesting. The C++ layer outputs an error message, and the Python layer deals with the error too, because presumably the error from calling PyHash from the C++ layer is still pending.

---

New commits:

e1374da

14211: catch Pynac exceptions

[rwst]

Changed commit from b9698e0 to e1374da

[jdemeyer]

comment:21

Replying to @rwst:

Interesting. The C++ layer outputs an error message, and the Python layer deals with the error too, because presumably the error from calling PyHash from the C++ layer is still pending.

Well, behaviour shouldn't depend on what "presumably" happens (does the traceback look sensible? If not, you're certainly doing it wrong). I admit I don't know the proper way to deal with this (a C++ exception and PyErr_Occurred() != NULL), but it's not right like this.

Besides that, I would never print the C++ exception.

[jdemeyer]

comment:23

I can have a look at how to properly deal with this, but not now.

[rwst]

Changed commit from e1374da to none

[rwst]

Changed author from Ralf Stephan to none

[rwst]

Changed branch from u/rws/14211 to none

[rwst]

comment:24

Now that the actual bug is fixed in future Pynac (#18360) the given doctest will not work in the future, so I removed the commit.

[rwst]

comment:25

Branch reset because other exceptions can be thrown.

---

New commits:

e1374da

14211: catch Pynac exceptions

[rwst]

Commit: e1374da

[rwst]

Branch: u/rws/14211

[rwst]

Changed commit from e1374da to none

[rwst]

Changed branch from u/rws/14211 to none

[rwst]

comment:26

No longer necessary (doctest added in #18360)