Fix PARI error handling

vbraun commented 11 years ago

Instead of catching whatever PARI writes to pariErr, we should use a callback function cb_pari_err_handle. That way, we don't need special tricks for warnings anymore.

Moreover, we also get access to the "error data" (a t_ERROR object) which we store in the exception.

Depends on #12142 Depends on #14873

CC: @jdemeyer

Component: packages: standard

Keywords: pari error signal

Author: Jeroen Demeyer

Branch/Commit: 2d532fc

Reviewer: Peter Bruin

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

vbraun commented 11 years ago

comment:2

Ah sorry, its dutch, not french. Too many variations of my own surname ;-)

pjbruin commented 11 years ago

comment:3

Replying to @vbraun:

Ah sorry, its dutch, not french. Too many variations of my own surname ;-)

Kein Problem! 8-)

pjbruin commented 11 years ago

Author: Peter Bruin

pjbruin commented 11 years ago

Dependencies: #12142, #14873

pjbruin commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,2 +1,39 @@
-Pari GIT has made significant changes to the error handling code, which will require us to make some changes to Sage. See also [#14873 comment:9](https://github.com/sagemath/sage/issues/14873#comment:9) for a list of issues with the current approach.
+Pari GIT has made significant changes to the error handling code, which will require us to make some changes to Sage.

+This ticket reimplements PARI error handling inside Cython code using macros provided by 'pari/paricom.h' instead of undocumented functions that no longer exist in PARI 2.6.  The new syntax is
+
+```
+sage_pari_catch()
+... code ...
+sage_pari_catch_end()
+```
+Inside such a block one can put conditions like
+
+```
+if [some condition]:
+    sage_pari_catch_reset()
+    [return or raise exception]
+```
+
+The new functions catch PARI errors and also call `sig_on()` and `sig_off()` to enable interrupt handling.  This replaces the current hack of overriding `sig_on()` and `sig_off()`.
+
+Since `sage_pari_catch()` and `sage_pari_catch_end()` are C macros, they `_must_` be on the same level in Cython code.  This means that several functions in `sage/libs/pari/gen.pyx`, most importantly `new_gen()`, no longer automatically call `sig_off()`.  Instead of
+
+```
+sig_on()
+return new_gen(something returning GEN)
+```
+one therefore has to write
+
+```
+cdef gen result
+sage_pari_catch()
+result = new_gen(something returning GEN)
+sage_pari_catch_end()
+return result
+```
+
+This new approach fixes several (potential) problems with the existing one; see [#14873 comment:9](https://github.com/sagemath/sage/issues/14873#comment:9).
+
+When upgrading to PARI 2.6, the only remaining change to be made (hopefully) is to replace the macros `CATCH`, `ENDCATCH`, `RETRY` and `CATCH_RELEASE` by `pari_CATCH`, `pari_ENDCATCH`, `pari_RETRY` and `pari_CATCH_reset`.
+

pjbruin commented 11 years ago

Changed keywords from none to pari error signal

pjbruin commented 11 years ago

new Cython macros for PARI error handling

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch.patch.gz

make gen.pyx use new PARI error handling macros

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch_gen.patch.gz

make PARI finite field elements use new error handling macros

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz

new PARI error handling for miscellaneous Cython code

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch_misc.patch.gz

delete old Cython macros for PARI error handling

pjbruin commented 11 years ago

Description changed:

--- 
+++ 
@@ -37,3 +37,4 @@

 When upgrading to PARI 2.6, the only remaining change to be made (hopefully) is to replace the macros `CATCH`, `ENDCATCH`, `RETRY` and `CATCH_RELEASE` by `pari_CATCH`, `pari_ENDCATCH`, `pari_RETRY` and `pari_CATCH_reset`.

+Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz),

pjbruin commented 11 years ago

comment:5

Attachment: trac_14894-pari_catch_delete_old.patch.gz

pjbruin commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 Pari GIT has made significant changes to the error handling code, which will require us to make some changes to Sage.

-This ticket reimplements PARI error handling inside Cython code using macros provided by 'pari/paricom.h' instead of undocumented functions that no longer exist in PARI 2.6.  The new syntax is
+This ticket reimplements PARI error handling inside Cython code using macros provided by `pari/paricom.h` instead of undocumented functions that no longer exist in PARI 2.6.  The new syntax is

sage_pari_catch() @@ -17,7 +17,7 @@

The new functions catch PARI errors and also call sig_on() and sig_off() to enable interrupt handling. This replaces the current hack of overriding sig_on() and sig_off().

-Since sage_pari_catch() and sage_pari_catch_end() are C macros, they _must_ be on the same level in Cython code. This means that several functions in sage/libs/pari/gen.pyx, most importantly new_gen(), no longer automatically call sig_off(). Instead of +Since sage_pari_catch() and sage_pari_catch_end() are C macros containing complementary braces, they must be on the same level in Cython code. This means that several functions in sage/libs/pari/gen.pyx, most importantly new_gen(), no longer call sig_off() or its replacement sage_pari_catch_end(). Instead of

 sig_on()
@@ -37,4 +37,4 @@

 When upgrading to PARI 2.6, the only remaining change to be made (hopefully) is to replace the macros `CATCH`, `ENDCATCH`, `RETRY` and `CATCH_RELEASE` by `pari_CATCH`, `pari_ENDCATCH`, `pari_RETRY` and `pari_CATCH_reset`.

-Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz), 
+Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz)

vbraun commented 11 years ago

comment:7

Looks fine to me. We lose some syntactic sugar but get closer to the upstream coding convention, so it should make our live easier in the long run. Maybe Jeroen has an opinion?

I would have preferred some shorter names especially since they'll be used very frequently in the Pari library interface. The file is part of the Sage library, so why prefix with sage_?. Maybe just catch_begin / catch_end / catch_reset since its usage should be pretty much only in sage/libs/pari.

pjbruin commented 11 years ago

comment:8

Replying to @vbraun:

Looks fine to me. We lose some syntactic sugar but get closer to the upstream coding convention, so it should make our live easier in the long run.

The new macros do require more keystrokes -- or things like replace-regexp in Emacs, which I ought to list as a co-author of these patches. 8-) On the other hand, they make the code easier to understand in the sense that one doesn't have to remember or guess where the error catching block ends.

I would have preferred some shorter names especially since they'll be used very frequently in the Pari library interface. The file is part of the Sage library, so why prefix with sage_?. Maybe just catch_begin / catch_end / catch_reset since its usage should be pretty much only in sage/libs/pari.

In fact, I spent quite a bit of thought on the names, and altogether, I think that there are good reasons for the sage_pari_ prefix:

My first idea was pari_catch / pari_endcatch / pari_catch_reset. Then I decided that this set of names looks better with catch_end than with endcatch.

The variable sage_pari_catcherr in your patch for #14873 then gave me the idea of prefixing the names with sage_. It is true that this makes them a bit long (which is why I did not change sage_pari_catch to sage_pari_catch_begin). But it is also true that they are non-trivial wrappers around the corresponding PARI macros that involve signal trapping, conversion of PARI errors to Python exceptions, and potentially retrying the same code several times until the stack is large enough. I thought this justified the prefix sage_ to make it clear that they are not the vanilla PARI macros but somewhat elaborate customised versions.

About keeping the pari part of the name: I think it is important because these macros are used outside sage/libs/pari, so the name should indicate that they have a more specialised purpose than the generic sig_on() / sig_off(). Currently they are used in a few places for integers, real and complex numbers, and integer and rational matrices. The FiniteField_pari_ffelt class of #12142 heavily depends on them, and there are many other data types of which fast PARI implementations could be made (e.g. polynomials, power series rings and elliptic curves over finite fields), which would undoubtedly also use these macros.

pjbruin commented 11 years ago

Description changed:

--- 
+++ 
@@ -37,4 +37,4 @@

 When upgrading to PARI 2.6, the only remaining change to be made (hopefully) is to replace the macros `CATCH`, `ENDCATCH`, `RETRY` and `CATCH_RELEASE` by `pari_CATCH`, `pari_ENDCATCH`, `pari_RETRY` and `pari_CATCH_reset`.

-Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz)
+Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz), [attachment: trac_14894-pari_catch_simplify.patch](https://github.com/sagemath/sage-prod/files/10658142/trac_14894-pari_catch_simplify.patch.gz)

pjbruin commented 11 years ago

comment:9

I'm attaching one more patch, which I hope will be the last one for this ticket. It makes the C macros as simple as possible and moves the handling of pari_errno to a single Cython function pari_handle_error.

pjbruin commented 11 years ago

comment:10

One other thing I experimented with is a decorator @pari_catch_function, which wraps the function in sage_pari_catch()...sage_pari_catch_end(). It works nicely, but I'm not sure that it is useful for gen.pyx, since it will probably slow things down. On the other hand, it might be useful later for new code that is less speed-critical. I am thinking of creating a patch implementing this (in a new ticket) sometime in the future.

jdemeyer commented 11 years ago

comment:11

One recomendation: could you use other names than sage_pari_catch, something which clearly refers to sig_on(), since sage_pari_catch() really is an extension of sig_on().

What about pari_sig_on() and pari_sig_off() which would mirror ecl_sig_on() and ecl_sig_off() which we already have?

jdemeyer commented 11 years ago

comment:12

Replying to @pjbruin:

One other thing I experimented with is a decorator @pari_catch_function, which wraps the function in sage_pari_catch()...sage_pari_catch_end().

Since this is low-level C code, I cannot believe a decorator (which is on the level of Python) would work.

jdemeyer commented 11 years ago

comment:13

Also, I really don't like that sage_pari_catch() and sage_pari_catch_end() contain complementaty braces. I would strongly recommend to try to keep the following working as before:

    sage_pari_catch()
    return new_gen(something returning GEN)

pjbruin commented 11 years ago

comment:14

Replying to @jdemeyer:

One recomendation: could you use other names than sage_pari_catch, something which clearly refers to sig_on(), since sage_pari_catch() really is an extension of sig_on().

As I understand it, the two purposes of sage_pari_catch() are equally important, namely

catching signals
catching PARI errors. It would be nice to have a name that mirror this. Maybe the prefix sage_ is somewhat redundant, as Volker Braun suggested, and we could call them pari_catch_on() and pari_catch_off()?

What about pari_sig_on() and pari_sig_off() which would mirror ecl_sig_on() and ecl_sig_off() which we already have?

Do these have the same two purposes as above? In that case, I would find the names pari_sig_on() and pari_sig_off() acceptable for the sake of consistency, but in principle I dislike the fact that they don't hint at the fact that these macros catch PARI errors and not just signals.

pjbruin commented 11 years ago

comment:15

Replying to @jdemeyer:

Since this is low-level C code, I cannot believe a decorator (which is on the level of Python) would work.

Maybe I can convince you by giving the definition of the decorator:

def pari_catch_function(f):
    """
    Decorator to wrap a function in sage_pari_catch() ... sage_pari_catch_end().
    """
    def g(*args):
        cdef object _x
        sage_pari_catch()
        _x = f(*args)
        sage_pari_catch_end()
        return _x
    return g

I put this in a Cython file and tested it. It works, but probably it is not very good for efficiency.

Something that I do not see how to do easily, on the other hand, is a context guard (context manager) for use with the with statement, i.e.

with pari_catch:
    ...code...

giving you catching of PARI errors and signals within the block.

pjbruin commented 11 years ago

comment:16

Replying to @jdemeyer:

Also, I really don't like that sage_pari_catch() and sage_pari_catch_end() contain complementaty braces. I would strongly recommend to try to keep the following working as before:
    sage_pari_catch()
    return new_gen(something returning GEN)

I see that this would be desirable because it saves typing a few lines each time. On the other hand, I think the above code is bad style because it explicity begins a block, but implicity ends it. The fact that the macros contain complementary braces makes them less flexible, but make it clearer where the error-catching block ends.

Actually, as far as I'm concerned it would be even nicer if we could have the with syntax as in my previous comment. This would force us at the same time to avoid complementary braces, which can be a good thing or not (we seem to disagree on this). However, there are two real disadvantages to this approach:

it won't translate into simple C code and will certainly be slower;
we cannot use the macros that PARI provides, and instead hack something ourselves that uses alloca(), malloc() or a global variable to allocate the jmp_buf (stack environment for use with sigjmp()); otherwise it will disappear from the scope. This is exactly the problem that this ticket tries to solve.

pjbruin commented 11 years ago

comment:17

One last comment for now: when I was working on this ticket I realised that a language feature that would give a good solution for this problem would be Lisp-style macros (so one could pass a "quoted" piece of code to a macro that does catch_on(), evaluates the code, and does catch_off()). Unfortunately this is something that Python doesn't have; the closest thing is the decorator described above, but it only works for entire functions.

vbraun commented 11 years ago

comment:18

I also don't like the unbalanced-bracket-hack. But, since Pari sets up its error catching framework that way, we should go with it, ugly or not.

Then I agree with Peter that the name should be different form sig_on/off precisely because it has different semantics.

I don't really see the need for getting too fancy with context managers or decorators, the pari error try/catch should essentially only be used in sage.libs.pari and nowhere else in the Sage tree. And that is precisely where you wouldn't want to take any performance hit for one of the more fancy solutions.

jdemeyer commented 11 years ago

comment:19

Replying to @pjbruin:

Actually, as far as I'm concerned it would be even nicer if we could have the with syntax as in my previous comment.

Sure, but that needs some Cython patches, so it's not possible now.

jdemeyer commented 11 years ago

comment:20

Replying to @pjbruin:

I put this in a Cython file and tested it. It works, but probably it is not very good for efficiency.

Since _x = f(*args) is evaluated through Python (not C), it will badly break the Python interpreter if interrupted at the wrong time.

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch_simplify.patch.gz

simplify macros, do pari_errno handling in gen.pyx:pari_handle_error

pjbruin commented 11 years ago

comment:21

In the previous version of attachment: trac_14894-pari_catch_simplify.patch I tried to move the except * from sage_pari_catch() to sage_pari_catch_end(). This was fine for catching PARI errors, but turned out to break interrupt handling, so I reverted this change.

pjbruin commented 11 years ago

comment:22

About the names for the macros: how about pari_catch_on() and pari_catch_off()?

Advantages:

not too long
reminiscent of sig_on() and sig_off()
do not suggest that the sole purpose is handling signals (as opposed to PARI errors)

jdemeyer commented 11 years ago

comment:23

Replying to @pjbruin:

reminiscent of sig_on() and sig_off()

In my opinion, not sufficiently reminiscent..., I still prefer pari_sig_on() and pari_sig_off().

I see that this would be desirable because it saves typing a few lines each time. On the other hand, I think the above code is bad style because it explicity begins a block, but implicity ends it.

That might be true, but in this case I don't think it justifies the overcomplication of two-liners like

sig_on() 
return P.new_gen(gcos(x.g, pbw(precision)))

to the five-liner

cdef gen result_gen 
sage_pari_catch() 
result_gen = P.new_gen(gcos(x.g, pbw(precision))) 
sage_pari_catch_end() 
return result_gen

jdemeyer commented 11 years ago

comment:24

For reviewing purposes, could you please fold all patches into one? Because some later patches rewrite stuff from older patches.

jdemeyer commented 11 years ago

comment:25

After looking at the PARI 2.6 sources, I think the best solution for Sage would be to use the (admittedly undocumented) variable iferr_env from PARI. Because the pari_CATCH stuff is re-doing a setjmp() call which Sage's sig_on() already does. This is bad, for example for performance reasons (I made a lot of effort in making sig_on() fast).

jdemeyer commented 11 years ago

Reviewer: Jeroen Demeyer

pjbruin commented 11 years ago

comment:27

Replying to @jdemeyer:

Replying to @pjbruin:

reminiscent of sig_on() and sig_off()

In my opinion, not sufficiently reminiscent..., I still prefer pari_sig_on() and pari_sig_off().

But why? When I started looking at gen.pyx, the names sig_on() and sig_off() really confused me, because they suggest that they are just signal handling macros. It seems to me that in practice, PARI errors (e.g. zero division) occur more often than signals, so the names should not suggest otherwise. If you ask me, this problem is not fixed by renaming them pari_sig_on() and pari_sig_off().

I see that this would be desirable because it saves typing a few lines each time. On the other hand, I think the above code is bad style because it explicity begins a block, but implicity ends it.

That might be true, but in this case I don't think it justifies the overcomplication of two-liners like [...] to the five-liner [...]

I can't say I'm completely happy with the code increase, either, and by now I'm really inclined to try one of two opposite strategies:

either find out how (in)efficient context managers (the with statement) are in Cython; I still think being able to say with pari_catch: code (as in comment:15) would be ideal, and perhaps the overhead is not so big compared to the overhead that we already have by using Cython;
or translate this part of gen.pyx from Cython to C, both for speed and to be able to use a macro for signal and error handling. This macro, say pari_catch_wrap(), should call pari_catch_on(), execute code (and remember its return value), call pari_catch_off() and return the return value of code, so one could type (in C)

return pari_catch_wrap(code);

After looking at the PARI 2.6 sources, I think the best solution for Sage would be to use the (admittedly undocumented) variable iferr_env from PARI. Because the pari_CATCH stuff is re-doing a setjmp() call which Sage's sig_on() already does. This is bad, for example for performance reasons (I made a lot of effort in making sig_on() fast).

There is a trade-off between execution time and developer time here.

What you suggest seems to require a new set of macros, merging sig_on()/sig_off() and PARI's error handling macros, that only do one sigsetjmp() but then have to distinguish within the error-handling code whether a signal or a PARI error occurred.

I would hope that setjmp() is very fast on most systems, so that it doesn't really hurt to use it twice: once to set up signal handling and once to set up PARI error handling. I would certainly be happy to pay this price in order to cleanly separate between handling signals and handling PARI errors.

jdemeyer commented 11 years ago

comment:28

Replying to @pjbruin:

either find out how (in)efficient context managers (the with statement) are in Cython; I still think being able to say with pari_catch: code would be ideal

Of course this would be ideal. But as I said, this needs Cython patches. I recently looked into this for implementing sig_on() and it currently cannot be done in Cython.

What you suggest seems to require a new set of macros, merging sig_on()/sig_off() and PARI's error handling macros, that only do one sigsetjmp() but then have to distinguish within the error-handling code whether a signal or a PARI error occurred.

Exactly. I know it's far from trivial, otherwise I would have done it already in the past. But I can say that I thought a lot about signal/error handling.

I would hope that setjmp() is very fast on most systems

It's not. I remember it being particularly slow on BSD (and therefore OS X) systems.

pjbruin commented 11 years ago

Attachment: trac_14894-pari_catch-folded.patch.gz

unified patch combining all the above patches

pjbruin commented 11 years ago

comment:29

Replying to @jdemeyer:

Replying to @pjbruin:

I would hope that setjmp() is very fast on most systems

It's not. I remember it being particularly slow on BSD (and therefore OS X) systems.

I just found a discussion about this on the pari-dev mailing list: http://pari.math.u-bordeaux.fr/archives/pari-dev-0909/msg00007.html.

The reason is that in BSD, setjmp() saves the signal mask (which apparently is very expensive), while in System V-like systems it does not. BSD does have _setjmp() and sigsetjmp(, 0), which are comparable to setjmp() on other systems. According to the link above, the setjmp() variant that does not touch the signal mask is 2.5 to 12 times as fast as the one that does.

Of course, for signal handling you are stuck with the slow version, but for other purposes one could use the fast version. In our situation the question is whether we do one slow system call, or the slow one for signal handling plus the fast one for handling PARI errors.

Finally, the discussion cited above mentions the idea of letting the user of the PARI library specify a function to call when an error occurs instead of the setjmp()/longjmp() construction. This doesn't currently seem to be implemented, though.

jdemeyer commented 11 years ago

comment:30

Replying to @pjbruin:

Of course, for signal handling you are stuck with the slow version

False :-) Sage's sig_on() uses sigsetjmp(env,0). It manually resets the signal mask when actually handling an error.

Finally, the discussion cited above mentions the idea of letting the user of the PARI library specify a function to call when an error occurs instead of the setjmp()/longjmp() construction.

I think this would be very good for Sage. I personally wouldn't mind patching that in ourselves in the Sage version of PARI.

jdemeyer commented 11 years ago

Description changed:

--- 
+++ 
@@ -37,4 +37,4 @@

 When upgrading to PARI 2.6, the only remaining change to be made (hopefully) is to replace the macros `CATCH`, `ENDCATCH`, `RETRY` and `CATCH_RELEASE` by `pari_CATCH`, `pari_ENDCATCH`, `pari_RETRY` and `pari_CATCH_reset`.

-Apply: [attachment: trac_14894-pari_catch.patch](https://github.com/sagemath/sage-prod/files/10658137/trac_14894-pari_catch.patch.gz), [attachment: trac_14894-pari_catch_gen.patch](https://github.com/sagemath/sage-prod/files/10658138/trac_14894-pari_catch_gen.patch.gz), [attachment: trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch](https://github.com/sagemath/sage-prod/files/10658139/trac_14894-pari_catch_FiniteFieldElement_pari_ffelt.patch.gz), [attachment: trac_14894-pari_catch_misc.patch](https://github.com/sagemath/sage-prod/files/10658140/trac_14894-pari_catch_misc.patch.gz), [attachment: trac_14894-pari_catch_delete_old.patch](https://github.com/sagemath/sage-prod/files/10658141/trac_14894-pari_catch_delete_old.patch.gz), [attachment: trac_14894-pari_catch_simplify.patch](https://github.com/sagemath/sage-prod/files/10658142/trac_14894-pari_catch_simplify.patch.gz)
+Apply: [attachment: trac_14894-pari_catch-folded.patch](https://github.com/sagemath/sage-prod/files/10658143/trac_14894-pari_catch-folded.patch.gz)

pjbruin commented 11 years ago

comment:32

Replying to @jdemeyer:

Replying to @pjbruin:

Of course, for signal handling you are stuck with the slow version

False :-) Sage's sig_on() uses sigsetjmp(env,0). It manually resets the signal mask when actually handling an error.

OK, I read the code too carelessly. But if you can simply use sigsetjmp(env, 0) for signal handling, then surely you can use it for handling PARI errors as well, so the slowness of setjmp() on BSD should be irrelevant. The try/catch macros provided by PARI do use a plain setjmp(), but that should be easy to change.

Anyway, if a non-setjmp()-based approach is feasible, that would be great. It would probably still be quite non-trivial, though.

jdemeyer commented 11 years ago

comment:33

Replying to @pjbruin:

But if you can simply use sigsetjmp(env, 0) for signal handling, then surely you can use it for handling PARI errors as well

Anyway, if a non-setjmp()-based approach is feasible, that would be great.

Both of these require patching the PARI sources, and then I prefer the second approach for simplicity.

pjbruin commented 11 years ago

comment:34

Actually, in [PARI source]/language/init.c (and in the header file paricom.h) there is a declaration

int  (*cb_pari_handle_exception)(long);

if this is non-zero, pari_err() calls it (with an error code as argument) when no saved environment is present to do a longjmp() to. I could imagine setting this to a Cython function that converts a PARI error into an appropriate Python exception, and raising that exception.

It appears that the function to which cb_pari_handle_exception points should return non-zero if and only if the error was handled succesfully; if it was, pari_err() simply returns. However, functions calling pari_err() normally expect that function not to return, so as things stand now, a suitable setjmp()/longjmp() would still seem to be necessary.

pjbruin commented 11 years ago

comment:35

And just now I discovered that as long ago as #9640, you (Jeroen) were already saying that we should use cb_pari_handle_exception to catch PARI errors...

jdemeyer commented 11 years ago

comment:36

In any case, is this urgent at all? I think it makes more sense to do this as part of the upgrade to PARI-2.6, because PARI-2.6 does change the error handling mechanism quite a bit.

pjbruin commented 11 years ago

comment:37

Replying to @jdemeyer:

In any case, is this urgent at all? I think it makes more sense to do this as part of the upgrade to PARI-2.6, because PARI-2.6 does change the error handling mechanism quite a bit.

The current implementation has its problems (see the ticket description), but I know of nothing that is broken in practice. Whether to do it now or during the upgrade depends on which solution is chosen.

Option 1: use the try/catch macros provided by PARI. This we can do now, and it will make the upgrade easier, since similar macros are provided in 2.6 (albeit with slightly different names and a different implementation).
Option 2: keep using undocumented PARI functions/variables. This can only be done at the same time as the upgrade, since the internals have changed a lot in 2.6. Option 1 is what the current patch does, and I still prefer this option.

jdemeyer commented 11 years ago

comment:38

Or option 3: patch PARI to hack the error handling functions.

jdemeyer commented 11 years ago

comment:39

Part of this should be done now for use in #14888: making the pari_sig_on() macros available, see #15124.

sagemath / sage

Fix PARI error handling #14894