Opened 11 years ago

Last modified 8 years ago

#9640 closed enhancement

Change PARI error catching mechanism — at Version 25

Reported by: jdemeyer Owned by: was
Priority: major Milestone: sage-5.13
Component: interfaces Keywords: pari error interrupt
Cc: leif, jdemeyer Merged in:
Authors: Peter Bruin, Jeroen Demeyer Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14029, #13311 Stopgaps:

Status badges

Description (last modified by jdemeyer)

Currently, the exceptions thrown by PARI are rather cryptic, like

Traceback (most recent call last):
...
PariError:  (15)

This ticket does the following:

  • Use cb_pari_handle_exception() instead of err_catch() to catch PARI exceptions.
  • Using a mechanism similar to #9636, catch the full text of the exception and pass it to PariError.

Apply: 9640-pari_error_callbacks_v2.patch

Change History (28)

comment:1 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 11 years ago by leif

  • Authors set to Jeroen Demeyer
  • Cc leif added

Changed 11 years ago by jdemeyer

Old patch for reference

comment:3 Changed 11 years ago by jdemeyer

  • Description modified (diff)
  • Keywords pari error interrupt added

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 8 years ago by pbruin

  • Cc pbruin added

comment:6 Changed 8 years ago by pbruin

I am working on a patch that does the following things suggested by the ticket description:

  • Rewrite pari_catch_sig_on() and pari_catch_sig_off() to use the callbacks cb_pari_handle_exception and cb_pari_err_recover (defined by the PARI library) in combination with sig_on()/sig_off(), as opposed to the additional setjmp() of the current implementation.
  • Catch output sent to pariErr and store it in a string attribute pari_instance.error_string for later use by PariError.

It seems to work quite well; the only doctest failures I've seen so far are a few tests expecting a PARI warning message. With the patch, these are not printed because they also go to pariErr.

To be done:

  • Make PariError aware of the stored string to give a more informative error message. This should probably simply be extracted from the lines starting with *** in pari_instance.error_string.
  • Minor change in PARI: send any extra newline at the start of the error message to pariErr instead of pariOut (this happens if the last printed character was not a newline).
  • Distinguish between errors and warnings; the latter should probably be printed to stderr as before. This should be done either by filtering the text sent to pariErr or by having pari_catch_sig_off() check if pari_instance.error_string is non-empty even if no error occurred.

This may make #14894 redundant, although that ticket could be used for general things to be done for the upgrade to PARI 2.6.

Changed 8 years ago by pbruin

based on 5.12.beta4 + #15124

comment:7 Changed 8 years ago by pbruin

  • Status changed from new to needs_review
  • Work issues set to see comment 6

The attached patch passes doctests on x86_64 GNU/Linux, except for two tests expecting a PARI warning (see above).

comment:8 Changed 8 years ago by pbruin

  • Status changed from needs_review to needs_work

comment:9 Changed 8 years ago by pbruin

  • Authors changed from Jeroen Demeyer to Peter Bruin
  • Cc jdemeyer added; pbruin removed

It would be better practice to move _pari_init_err(), _pari_handle_exception() and _pari_err_recover() from pari_err.h to a C file or to translate them to Cython. At the moment I don't think it is worth the trouble.

comment:10 Changed 8 years ago by pbruin

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues see comment 6 deleted

The latest patch does everything that I think it should do. I hope it is reasonably self-explanatory. A few remarks:

  • The error text is now passed to PariError and can be accessed using PariError.errtext(); is is not printed automatically. Using this text to improve messages like PariError: (15) will probably have to wait until another ticket. (Edit: user errors are an exception, since they are converted into RuntimeError; here we append the error text to the currently used "PARI user exception".)
  • There is no problem with PARI sending escape sequences for colours in error messages; this is disabled by default.
  • The printing of a possible extra newline before the error message is easily avoided using pari_set_last_newline(1).
  • The callback functions are now Cython functions in the new file pari_error.pyx. It is important that this does not include pari_err.pxi; because of the way Cython imports functions, pari_err.pxi and pari_err.h use function pointers with the same names as the functions defined in pari_error.pyx.
Last edited 8 years ago by pbruin (previous) (diff)

comment:11 Changed 8 years ago by pbruin

Patchbot:

apply 9640-pari_error_callbacks.patch​

comment:12 follow-up: Changed 8 years ago by jdemeyer

Why not set cb_pari_handle_exception = _pari_handle_exception globally and have pari_catch_sig_on() simply be equal to sig_on() (and then of course change pari_catch_sig_on() back to sig_on()).

comment:13 follow-up: Changed 8 years ago by jdemeyer

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

comment:14 in reply to: ↑ 12 Changed 8 years ago by pbruin

Replying to jdemeyer:

Why not set cb_pari_handle_exception = _pari_handle_exception globally and have pari_catch_sig_on() simply be equal to sig_on() (and then of course change pari_catch_sig_on() back to sig_on()).

Good question. That would mean we simply always have PARI exception handling enabled, which would be more efficient and should not make any difference since PARI shouldn't be called outside of sig_on()/sig_off() anyway. I'll try it out.

comment:15 follow-up: Changed 8 years ago by jdemeyer

Anyway, I am thinking about a review patch so I will make these changes myself.

Peter, is there any chance you could review #14029 and #13311 because those tickets also make changes to the interrupt code.

comment:16 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by pbruin

Replying to jdemeyer:

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

Why abort() and not raise(SIGUSR1) or another signal code?

comment:17 Changed 8 years ago by jdemeyer

I am thinking about merging (parts of) #10018 here, especially the doctests.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by jdemeyer

Replying to pbruin:

Replying to jdemeyer:

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

Why abort() and not raise(SIGUSR1) or another signal code?

Because we already handle abort() anyway and because I see no gain of adding an extra signal, with the risk that SIGUSR1 is used by other Sage packages.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 8 years ago by pbruin

Replying to jdemeyer:

Replying to pbruin:

Why abort() and not raise(SIGUSR1) or another signal code?

Because we already handle abort() anyway and because I see no gain of adding an extra signal, with the risk that SIGUSR1 is used by other Sage packages.

But we handle it by raising a new RuntimeError, among other things, unless I misunderstand interrupt.c. So wouldn't the SIGABRT handler need to have a special case for the case where it is called in this way? This would be rather inelegant.

comment:20 in reply to: ↑ 19 Changed 8 years ago by jdemeyer

Replying to pbruin:

This would be rather inelegant.

I personally don't find it inelegant. In any case, we would encapsalate this through some call like sig_error().

comment:21 in reply to: ↑ 15 Changed 8 years ago by pbruin

Replying to jdemeyer:

Peter, is there any chance you could review #14029 and #13311 because those tickets also make changes to the interrupt code.

Not very soon, unfortunately (lots of work at the moment), but I'll put my name in the CC list.

comment:22 Changed 8 years ago by jdemeyer

  • Dependencies set to #14029, #13311
  • Status changed from needs_review to needs_work

comment:23 Changed 8 years ago by jdemeyer

Working/thinking about this...

Changed 8 years ago by jdemeyer

based on 5.12.beta4 + #15124

comment:24 Changed 8 years ago by jdemeyer

  • Authors changed from Peter Bruin to Peter Bruin, Jeroen Demeyer
  • Description modified (diff)

comment:25 Changed 8 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.