Opened 8 months ago

Closed 6 months ago

#32602 closed defect (fixed)

Magma interface throws segfault

Reported by: kedlaya Owned by:
Priority: major Milestone: sage-9.5
Component: interfaces Keywords: Magma, booleans
Cc: Merged in:
Authors: Kiran Kedlaya Reviewers: Nils Bruin, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 46d7eac (Commits, GitHub, GitLab) Commit: 46d7eac7c6b2ca798bcaa958043478c31b18b05a
Dependencies: Stopgaps:

Status badges

Description

The following call to the Magma interface seems to be triggering a segfault on the Python side. Traceback is from 9.2, but I actually first hit this in 9.5beta1, so I assume nothing changed in between.

sage: Q.<x> = PolynomialRing(GF(3))
sage: u = x^6+x^4+2*x^3+2*x+1
sage: F0 = magma.FunctionField(GF(3))
sage: S = magma.PolynomialRing(F0)
sage: F = magma.FunctionField(S.1^2 - u(F0.1))
Fatal Python error: Cannot recover from stack overflow.
Python runtime state: initialized

Thread 0x00007f79c3d44700 (most recent call first):
  File "/usr/lib/python3.8/threading.py", line 302 in wait
  File "/usr/lib/python3.8/threading.py", line 558 in wait
  File "/scratch/sage/sage-9.2/local/lib/python3.8/site-packages/IPython/core/history.py", line 829 in run
  File "/scratch/sage/sage-9.2/local/lib/python3.8/site-packages/IPython/core/history.py", line 58 in needs_sqlite
  File "</scratch/sage/sage-9.2/local/lib/python3.8/site-packages/decorator.py:decorator-gen-24>", line 2 in run
  File "/usr/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/usr/lib/python3.8/threading.py", line 890 in _bootstrap

Current thread 0x00007f79c5cbf740 (most recent call first):
  File "/usr/lib/python3.8/contextlib.py", line 86 in __init__
  File "/usr/lib/python3.8/contextlib.py", line 240 in helper
...

Change History (13)

comment:1 Changed 8 months ago by chapoton

In the last line, already

u(F0.1)

triggers a

Fatal Python error: _Py_CheckRecursiveCall: Cannot recover from stack overflow.
Python runtime state: initialized
Version 1, edited 8 months ago by chapoton (previous) (next) (diff)

comment:2 Changed 8 months ago by kedlaya

For comparison, this works just fine:

sage: Q.<x,y> = PolynomialRing(GF(3), 2)
sage: u = x^6+x^4+2*x^3+2*x+1
sage: F0 = magma.FunctionField(GF(3))
sage: u(F0.1,0)
$.1^6 + $.1^4 + 2*$.1^3 + 2*$.1 + 1

so it is possible the underlying issue isn't in the interface at all.

comment:3 Changed 8 months ago by nbruin

The GDB attachment doesn't quite work for me, but it can print a traceback that gives some information:

...[loads of repeats START]
#11115 0x00007f618907563a in PyObject_IsTrue.part.0 ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11116 0x00007f618910f939 in bool_new () from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11117 0x00007f6189068e8b in type_call () from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11118 0x00007f6189068c73 in _PyObject_MakeTpCall ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11119 0x00007f61890650dd in _PyEval_EvalFrameDefault ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11120 0x00007f618906d8db in function_code_fastcall ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11121 0x00007f61890764d4 in method_vectorcall ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11122 0x00007f6189064fce in _PyEval_EvalFrameDefault ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11123 0x00007f618906d8db in function_code_fastcall ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11124 0x00007f6189097063 in PyObject_CallOneArg.lto_priv.13 ()
   from /lib64/libpython3.9.so.1.0
No symbol table info available.
#11125 0x00007f61890ce131 in slot_nb_bool () from /lib64/libpython3.9.so.1.0
No symbol table info available.
...[loads of repeats END]

#11138 0x00007f61791f949a in __pyx_pf_4sage_9structure_7element_7Element_60is_zero (__pyx_v_self=<optimized out>)
    at build/cythonized/sage/structure/element.c:10177
...
#11142 0x00007f60159e8b08 in __pyx_pf_4sage_5rings_10polynomial_18polynomial_element_10Polynomial_18__call__ (__pyx_v_kwds=0x7f5fc9b275c0, 
    __pyx_v_args=0x7f5fc9b10f70, __pyx_v_self=<optimized out>)
    at build/cythonized/sage/rings/polynomial/polynomial_element.c:9783
...
#11143 __pyx_pw_4sage_5rings_10polynomial_18polynomial_element_10Polynomial_19__call__ (__pyx_v_self=<optimized out>, __pyx_args=0x7f5fc9b10f70, 
    __pyx_kwds=<optimized out>)
    at build/cythonized/sage/rings/polynomial/polynomial_element.c:8807
...
#11146 __pyx_pf_4sage_5rings_10polynomial_21polynomial_zmod_flint_21Polynomial_zmod_flint_4__call__ (__pyx_v_kwds=0x7f5fc9ad4fc0, __pyx_v_x=0x7f5fc9b10a90, 
    __pyx_v_self=<optimized out>)
    at build/cythonized/sage/rings/polynomial/polynomial_zmod_flint.cpp:15823
 ....
#11147 __pyx_pw_4sage_5rings_10polynomial_21polynomial_zmod_flint_21Polynomial_zmod_flint_5__call__ (__pyx_v_self=<optimized out>, __pyx_args=0x7f5fc9b10a90, 
    __pyx_kwds=<optimized out>)
    at build/cythonized/sage/rings/polynomial/polynomial_zmod_flint.cpp:15409
 ...

It looks like this traceback goes inner-at-top, so it seems there is some zmod_flint that leads to an infinite recursion in determining if something is true.

In the python traceback there is a pretty good indication of where the infinite recursion happens:

...
Current thread 0x00007f6dbad68740 (most recent call first):
  File "/usr/lib64/python3.9/re.py", line 291 in _compile
  File "/usr/lib64/python3.9/re.py", line 252 in compile
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/pexpect/spawnbase.py", line 232 in compile_pattern_list
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/pexpect/spawnbase.py", line 342 in expect
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/expect.py", line 983 in _eval_line
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/expect.py", line 1381 in <listcomp>
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/expect.py", line 1381 in eval
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/magma.py", line 558 in eval
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/magma.py", line 1080 in _next_var_name
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/interface.py", line 512 in _create
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/expect.py", line 1469 in __init__
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/interface.py", line 295 in __call__
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/magma.py", line 791 in __call__
...[The following repeat a LOT]
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/magma.py", line 2631 in __bool__
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/interface.py", line 1308 in bool
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/magma.py", line 2631 in __bool__
  File "/usr/local/sage/sage-git/local/lib64/python3.9/site-packages/sage/interfaces/interface.py", line 1308 in bool

so this looks like a __bool__ call on a magma interface element leading to exactly the same again (probably with the same argument). That's consistent with the GDB traceback part too (at least the fact that PyObject_IsTrue is involved)

comment:4 Changed 8 months ago by nbruin

Indeed,

sage: Q.<x> = PolynomialRing(GF(3)) 
sage: u = x^6+x^4+2*x^3+2*x+1 
sage: F0 = magma.FunctionField(GF(3))                                                                   
sage: bool(F0.1)

already blows up.

In fact:

sage: magma("1 eq 0").bool()

blows up: the magma interface cannot determine the truth value of magma's "false" object.

The same happens for

sage: magma("1 eq 1").bool()

On can see why: looking into the the code, we're executing:

        try:
            return not self.parent()("%s eq 0" % self.name()).bool()
        except TypeError:
            # comparing with 0 didn't work; try comparing with
            try:
                return not self.parent()("%s eq false" % self.name()).bool()

so in order to determine the truth value of "true", we first try "true eq 0" (which raises TypeError?) and then try "true eq false" which will produce "false". We then try "false eq 0" and then "false eq false" which will produce "true", and now the circle is complete.

This has never worked. I suspect someone was a bit overzealous in changing a "x == 0" to a "bool(x)", because that's something that python allows but isn't always a better expression of intent and, as this shows, may not actually be an improvement in code because it's not actually equivalent!!! F0.1 == 0 works just fine.

Last edited 8 months ago by nbruin (previous) (diff)

comment:5 Changed 8 months ago by nbruin

Looks like this goes back to https://trac.sagemath.org/ticket/7504 . I suspect that with the transition from Py2 to Py3 and the corresponding change from __nonzero__ to __bool__ a delicate balance was disturbed that previously allowed boolean objects from escaping this infinite recursion. Clearly, the results of these "%s eq 0" and "%s eq false" tests should be examined for their string value and not depend on reentering the interface code to re-apply "bool".

comment:6 Changed 8 months ago by kedlaya

Well, that explains my feeling of deja vu anyway!

comment:7 Changed 7 months ago by kedlaya

  • Branch set to u/kedlaya/magma_interface_throws_segfault

comment:8 Changed 7 months ago by kedlaya

  • Authors set to Kiran Kedlaya
  • Commit set to c896669b1f2303da42e33bbb50d664bd16c7124c
  • Keywords booleans added
  • Status changed from new to needs_review

Patch to change the __bool__ method as nbruin suggests, doing the comparison at the level of the Magma string representation. I'm a bit nervous that this is too simple-minded, but it doesn't seem to break any of the doctests.

Speaking of which, I tweaked a couple of unrelated doctests which were slightly off (possibly dependent on Magma version; I tried this with 2.25-5).

comment:9 Changed 7 months ago by nbruin

It looks like the branch linked presently is just 9.5.beta3. Did you forget to push?

comment:10 Changed 7 months ago by git

  • Commit changed from c896669b1f2303da42e33bbb50d664bd16c7124c to 46d7eac7c6b2ca798bcaa958043478c31b18b05a

Branch pushed to git repo; I updated commit sha1. New commits:

46d7eacChange __bool__ in Magma interface

comment:11 Changed 7 months ago by kedlaya

I had remembered to push but not to commit. :)

comment:12 Changed 7 months ago by tscrim

  • Reviewers set to Nils Bruin, Travis Scrimshaw
  • Status changed from needs_review to positive_review

The fix seems reasonable to me. Nils, if you have any further comments, feel free to add them (including reverting the positive review).

comment:13 Changed 6 months ago by vbraun

  • Branch changed from u/kedlaya/magma_interface_throws_segfault to 46d7eac7c6b2ca798bcaa958043478c31b18b05a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.