Possible future issues with ECL build with unicode enabled

[pcpa]

ecl 12.2.1 has unicode support enabled by default, what creates two string types, t_base_string and t_string. The problem is that sage does not properly handle those, as described in  https://sourceforge.net/tracker/?func=detail&aid=3526370&group_id=30035&atid=398053 see also  https://github.com/cschwan/sage-on-gentoo/issues/2

AFAIK it can be built with unicode disabled, just that it will fail make check at least.

Apply:

• trac12985-unicode.patch

[pcpa]

sage-5.0-ecl-unicode.patch

[fbissey]

Thanks Paulo, we had identified that problem in sage-on-gentoo a long time ago but since we can just ask for ecl to be build without unicode support for sage we just did that. If we apply your patch does sage work just as well if ecl is compiled without unicode support or does that become a requirement?

[fbissey]

Actually just looking at the patch, you are just adding a conversion layer, right?

[pcpa]

Replying to fbissey:

Actually just looking at the patch, you are just adding a conversion layer, right?

Yes, and if the ecl is built without unicode support, it should be a minor overhead noop, as it would attempt to convert a base string to a base string :-) There are some comments from Juan at  http://sourceforge.net/tracker/?func=detail&aid=3526370&group_id=30035&atid=398053 that may be of interest, about possible issues with unprotected lisp objects.

[nbruin]

Juan's of course right in being concerned that objects are protected against garbage collection while alive. As far as I know, Boehm does watch the C stack for pointers, so a local variable pointing to the string should protect it against garbage collection while in scope.

I'm pretty sure that every ecl_base_string_pointer_safe occurs in a context where a python string object is required. Cython injects a PyBytes_FromString for that, which copies the content into a Python allocated object. The ECL-managed object is thus extremely short lived and during its life, is referenced by a local C variable (i.e., is referenced on the C stack). Boehm is supposed to watch the C stack for references, so it is protected against GC during its life.

If the reference returned by ecl_base_string_pointer_safe were to be stored in, say, a malloc'd location (or Python managed memory) such protection is no longer guaranteed. This is why EclObject.set_obj makes the little remove_node/insert_node_after dance, which ensures that a reference to the underlying ecl-object is stored within memory watched by Boehm-GC.

EDIT: Actually, I see what Juan is worried about: ecl_base_string_pointer_safe is a pointer to the string content, not the ecl string object. So yes, explicitly assigning the result of cl_write_to_string to a local variable, as he suggests, is probably a good idea.

The whole unicode problem would of course be much more elegantly solved by directly converting ecl unicode strings to python unicode strings and back. Given our use of ecl (maxima), there would be very little actual gain from doing so.

[fbissey]

patch to wrap ecl potential unicode output

[fbissey]

I have put the patch in a better shape and checked it applied correctly on a recent 5.2beta0. I'd very much like this to go in 5.2. It doesn't really benefit sage at the present time but makes the works from people like me and Paulo on getting sage to run natively on linux distros much easier. I am doing a few tests and then I'll give it the thumbs up.

[fbissey]

Paulo I tried the patch with 5.1.beta6 and ecl-12.2.1 with unicode enabled and the sage/libs/ecl.pyx doctest stops waiting for input (with -verbose I get):

Trying:
    interrupt_after_delay(Integer(1000))###line 246:_sage_    >>> interrupt_after_delay(1000)
Expecting nothing
ok
Trying:
    inf_loop()###line 247:_sage_    >>> inf_loop()
Expecting:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt

Condition of type: SIMPLE-TYPE-ERROR
In function MAKE-FOREIGN-DATA-FROM-ARRAY, the value of argument is
        "Console interrupt."
which is not of expected type BASE-STRING
Available restarts:

1. (USE-VALUE) Supply a new value of type BASE-STRING.

Top level.
> 

and it stays at that prompt until I interrupt it.

[pcpa]

I cannot test it right now as I am still fighting to get a lot of packages updated in fedora, but will try to switch from sage-5.0.1 to 5.1.beta as soon as possible. The failure could be the requirement of some extra patch, or, the "interrupt after delay" may need some kind of "non interrruptible" code block when making the conversion.

But the error message appears to tell it somehow got a t_string where a t_base_string was expected, so, probably the ecl-unicode patch needs to be extended for 5.1.beta6.

Hopefully, sage/libs/ecl.pxd diff from 5.1.beta6 to 5.0.1 will show where the probem is, or, a new test case is triggering some unexpected condition.

[nbruin]

Isn't this just a case of missed conversions? On line 256 we have

    if ecl_nvalues > 1:
        raise RuntimeError, "ECL says: "+ecl_base_string_pointer_safe(ecl_values(1))
    else:
        return ecl_values(0)

(this occurs in ecl_safe_eval, ecl_safe_funcall, ecl_safe_apply) and none of the patches here seem to touch that line. The second value will be ECL's error string, which is probably a unicode string if ECL uses unicode. Two solutions:

• Change the definitions of the LISP routines sage-safe-eval and friend (line 212 and further) to already convert the error message to base_string, e.g.:

        (defun sage-safe-eval (form)
            (handler-case
                (values (eval form))
                (serious-condition (cnd)
                    (values nil (si:coerce-to-base-string (princ-to-string cnd))))))

I don't think in this case we need to store the reference to ecl_values(1), because the value itself is in a "last value returned" buffer by ecl and all we're doing is reaching into it to get a string pointer and copy it into a python string. No opportunity for ecl to invalidate the values buffer.

• do the conversion in the python routines:

cdef cl_object ecl_safe_eval(cl_object form) except NULL:
    cdef cl_object s 
    ecl_sig_on()
    cl_funcall(2,safe_eval_clobj,form)
    ecl_sig_off()

    if ecl_nvalues > 1:
        s = si_coerce_to_base_string(ecl_values(1))
        raise RuntimeError, "ECL says: "+ecl_base_string_pointer_safe(s)
    else:
        return ecl_values(0)

[pcpa]

Rediff of patch and applying changes in nbruin comment

[fbissey]

Replying to fbissey:

Paulo I tried the patch with 5.1.beta6 and ecl-12.2.1 with unicode enabled and the sage/libs/ecl.pyx doctest stops waiting for input (with -verbose I get):

Trying:
    interrupt_after_delay(Integer(1000))###line 246:_sage_    >>> interrupt_after_delay(1000)
Expecting nothing
ok
Trying:
    inf_loop()###line 247:_sage_    >>> inf_loop()
Expecting:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt

Condition of type: SIMPLE-TYPE-ERROR
In function MAKE-FOREIGN-DATA-FROM-ARRAY, the value of argument is
        "Console interrupt."
which is not of expected type BASE-STRING
Available restarts:

1. (USE-VALUE) Supply a new value of type BASE-STRING.

Top level.
> 

and it stays at that prompt until I interrupt it.

With ecl-12.7.1 it just hangs at the same spot even without unicode so there may be something deeper going on there. Haven't tried the new patch it did go under my radar unnoticed so I don't know if it will solve the problem yet.

[fbissey]

I will have to reformat your patch a little bit Paulo. Not only it wasn't produced by hg my cython didn't like the result very much

Error compiling Cython file:
------------------------------------------------------------
...
    ecl_sig_on()
    cl_funcall(2,safe_eval_clobj,form)
    ecl_sig_off()

    if ecl_nvalues > 1:
        s = si_coerce_to_base_string(ecl_values(1))
^
------------------------------------------------------------

sage/libs/ecl.pyx:258:0: Mixed use of tabs and spaces

[fbissey]

I have a riddle. With the patch and ecl 12.7.1

sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx" -verbose

take 2.9s and is successful if you exclude  this little problem but if I run it without "-verbose" it times out - presumably at the same spot. And I haven't tried unicode yet.

[pcpa]

I noticed later I submitted a patch with tabs instead of spaces in a few places but left it as an exercise for the reader to correct :-)

It appears to work in my current Fedora build

$ rpm -q sagemath ecl
sagemath-5.2-1.fc18.x86_64
ecl-12.2.1-4.fc18.x86_64

Running it I see:

$ sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"
sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"     
**********************************************************************
File "/usr/share/sagemath/devel/sage/sage/libs/ecl.pyx", line 247:
    sage: inf_loop()
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt
Got:
    Traceback (most recent call last):
      File "/usr/share/sagemath/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/share/sagemath/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/share/sagemath/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_6[7]>", line 1, in <module>
        inf_loop()###line 247:
    sage: inf_loop()
      File "ecl.pyx", line 712, in sage.libs.ecl.EclObject.__call__ (sage/libs/ecl.c:5042)
      File "ecl.pyx", line 285, in sage.libs.ecl.ecl_safe_apply (sage/libs/ecl.c:3047)
    RuntimeError: ECL says: Console interrupt.
**********************************************************************
1 items had failures:
   1 of   9 in __main__.example_6
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/pcpa/.sage/tmp/ecl_1962.py
         [7.1 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"
Total time for all tests: 7.2 seconds

Also takes 2.8s in the second run...

I suspect it should be ecl-2.7.1, but there may be a few other variables involving gc and glibc version.

I did not look at ecl-2.7.1 yet, but maybe need a patch equivalent to

 http://pkgs.fedoraproject.org/cgit/ecl.git/tree/ecl-12.2.1-signal_handling_thread.patch

Should not be required as sagemath should already have an equivalent patch, and the above patch was required to make maxima ecl backend not hang.

[fbissey]

We don't that patch in Gentoo at all. Never noticed a problem with maxima so far. It is just curious that with 12.7.1 I get a different behavior between a verbose and non-verbose test. No that's a puzzler to me. There must be a variable affecting ecl or something.

[fbissey]

OK your patch now works as expected without fuss with ecl-12.2.1. I'll reformat the patch for inclusion in sage and save the weird behavior in 12.7.1 for another day. Sounds OK?

[fbissey]

latest version of Paulo

[fbissey]

Re-generated the patch against sage-5.3.beta0. Pushing the positive review button.

[pcpa]

Replying to fbissey:

OK your patch now works as expected without fuss with ecl-12.2.1. I'll reformat the patch for inclusion in sage and save the weird behavior in 12.7.1 for another day. Sounds OK?

It is ok with me. I should test latest upstream ecl at some point also. BTW, the ecl-12.2.1-signal_handling_thread.patch in Fedora was my solution to allow maxima's make check to work with the ecl backend, when I made patches and requested support for the ecl backend; most tests would work, I think it was that it would fail somewhere in the tests, probably due to triggering gc in some specific time window.

[fbissey]

Wait a minute we cannot merge this if we don't have ecl compiled with unicode.

[pcpa]

Replying to fbissey:

Wait a minute we cannot merge this if we don't have ecl compiled with unicode.

It is supposed to be a noop if ecl is built without unicode support. AFAIR I made tests with unicode disabled and applying the propsed patch, but it should be wise to test it again.

[fbissey]

It causes problem for me with

sage -t -long  -force_lib devel/sage-main/sage/calculus/calculus.py

[?1034h#0: solve_rat_ineq(ineq=x # 5)
#0: simplify_sum(expr='sum(q^k,k,0,inf))
#1: simplify_sum(expr=a*'sum(q^k,k,0,inf))
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/calculus/calculus.py", line 677:
    sage: f.nintegral(x,0,1,1e-14)
Exception raised:
    Traceback (most recent call last):
      File "/usr/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_3[3]>", line 1, in <module>
        f.nintegral(x,Integer(0),Integer(1),RealNumber('1e-14'))###line 677:
    sage: f.nintegral(x,0,1,1e-14)
      File "expression.pyx", line 9073, in sage.symbolic.expression.Expression.nintegral (sage/symbolic/expression.cpp:38115)
      File "/usr/lib64/python2.7/site-packages/sage/calculus/calculus.py", line 752, in nintegral
        raise ValueError, "Maxima (via quadpack) cannot compute the integral"
    ValueError: Maxima (via quadpack) cannot compute the integral
**********************************************************************
1 items had failures:
   1 of  17 in __main__.example_3
***Test Failed*** 1 failures.

May be your maxima patch deals with that. If I use an ecl compiled without unicode and this patch at the same time we there are unresolved symbol because of the declaration

cl_object si_coerce_to_base_string(cl_object x) 

in ecl.pxd. It won't be there if ecl is compiled without unicode. I may have to crosscheck that if that particular bit is nasty or not.

[pcpa]

Sage and maxima must link to an ecl with the same build options. From /usr/include/ecl/external.h

#ifdef ECL_UNICODE
extern ECL_API cl_object si_base_char_p(cl_object x);
extern ECL_API cl_object si_base_string_p(cl_object x);
extern ECL_API cl_object si_coerce_to_base_string(cl_object x);
extern ECL_API cl_object si_coerce_to_extended_string(cl_object x);
#define ecl_alloc_simple_extended_string(l) ecl_alloc_simple_vector((l),aet_ch)
extern ECL_API cl_object ecl_alloc_adjustable_extended_string(cl_index l);
#else
#define si_base_char_p cl_characterp
#define si_base_string_p cl_stringp
#define si_coerce_to_base_string cl_string
#define si_coerce_to_extended_string cl_string
#endif

[fbissey]

That would be why I got bad stuff about unresolved symbols with ecl.pyx but for the calculus.py doctest I had something consistent: ecl with unicode and maxima and sage rebuilt against it.

[kcrisman]

The current packages at #13324 have

--enable-unicode=no

Would that be an acceptable resolution to this ticket, or should we re-enable it with this fix eventually? (Currently I'd say #13324 is higher-priority so that we might actually have a chance at getting Cygwin up to speed...)

[pcpa]

Replying to kcrisman:

The current packages at #13324 have --enable-unicode=no

The fedora package has an explicit --enable-unicode. The fact that ecl was failing make check if using --disable-unicode was an ecl bug.

Would that be an acceptable resolution to this ticket, or should we re-enable it with this fix eventually? (Currently I'd say #13324 is higher-priority so that we might actually have a chance at getting Cygwin up to speed...)

The patch must work with ecl compiled with or without unicode (the patch reason is to not break sagemath if ecl was built with unicode enabled), but the patch was causing a link failure if using an ecl older than 12.2.

[fbissey]

I'll have to check if I still have a couple of weird issues stemming from Unicode and this patch. Haven't touched it in while. I have been more focused on upgrading numpy in #11334 of late.