Opened 9 years ago
Closed 6 years ago
#10018 closed defect (fixed)
Improve PARI's stack allocation
Reported by: | jdemeyer | Owned by: | jdemeyer |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.13 |
Component: | c_lib | Keywords: | pari init_stack |
Cc: | leif | Merged in: | sage-5.13.beta4 |
Authors: | Jeroen Demeyer | Reviewers: | Peter Bruin |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #9640 | Stopgaps: |
Description (last modified by )
Make the PARI stack allocation more robust. In particular, if doubling fails, try to increase the stack by a smaller amount. And check for overflow in init_stack()
.
Also make the default PARI stack size a lot smaller (1MB instead of 16MB).
Attachments (3)
Change History (35)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Keywords allocatemem added
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Cc leif added
comment:5 Changed 9 years ago by
- Description modified (diff)
comment:6 Changed 9 years ago by
- Description modified (diff)
- Keywords interrupt _sig_on _sig_retry added; bnfcertify removed
comment:7 follow-up: ↓ 8 Changed 9 years ago by
A good place to also add some more case distinctions on PARI's errnum
... ;-)
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 9 years ago by
comment:9 Changed 9 years ago by
- Description modified (diff)
- Owner changed from tba to jdemeyer
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 9 years ago by
Replying to jdemeyer:
Replying to leif:
A good place to also add some more case distinctions on PARI's
errnum
... ;-)This is planned for #9640 (in the not-so-near future though).
Ok. I thought of just adding number 5 (talker
?) here, mapping it to ValueError
, which comes nearest (could also be TypeError
(domain) or RangeError
etc.).
comment:11 Changed 9 years ago by
... because of #9640 "not-so-near future".
comment:12 in reply to: ↑ 10 ; follow-up: ↓ 13 Changed 9 years ago by
Replying to leif:
Ok. I thought of just adding number 5 (
talker
?) here, mapping it toValueError
, which comes nearest (could also beTypeError
(domain) orRangeError
etc.).
I think it is good that now all errors thrown by PARI are of the type PariError
. If you want to raise a different exception, I think it should be done by higher-level code, like:
try: # do something with PARI except PariError, err: if err.errnum() == talker: raise ValueError raise err
(this currently only works with Cython because talker
is defined in a PARI C header).
comment:13 in reply to: ↑ 12 ; follow-ups: ↓ 14 ↓ 15 Changed 9 years ago by
Replying to jdemeyer:
(this currently only works with Cython because
talker
is defined in a PARI C header).
... and would require changing much more code. ;-)
The actual error message should be vsnprintf()
'ed into a buffer (upstream) such that we can also propagate the real cause.
comment:14 in reply to: ↑ 13 Changed 9 years ago by
Replying to leif:
The actual error message should be
vsnprintf()
'ed into a buffer (upstream) such that we can also propagate the real cause.
That is more or less the idea of #9640.
But I also noticed that the whole exception-catching mechanism can be vastly improved (for example, now there is a general signal-handling facility in Sage and a specific error-handling facility for PARI which should be merged into one system). Also, there is essentially no testing code for this Cython error handling. Good testing code should have caught #10018. All this should eventually be in #9678.
comment:15 in reply to: ↑ 13 Changed 9 years ago by
Replying to leif:
The actual error message should be
vsnprintf()
'ed into a buffer (upstream) such that we can also propagate the real cause.
I added the following in src/language/init.c
to debug PARI:
void pari_err(int numerr, ...) { PariOUT *out = pariOut; va_list ap; va_start(ap,numerr); if (numerr==talker) /* added part */ { va_list ap2; const char *fmt; va_copy(ap2,ap); fmt=(char*)va_arg(ap2,char*); vfprintf(stderr,fmt,ap2); /* we can equally well write into a global buffer here */ fflush(stderr); /* with vsnprintf() to propagate the error message */ } ...
comment:16 Changed 9 years ago by
I actually added some more printf()
s:
... fprintf(stderr, "\n******************** PARI ERROR: ********************\n"); vfprintf(stderr,fmt,ap2); fprintf(stderr, "\n*****************************************************\n"); fflush(stderr); ...
Of course a temporary hack (and a bit odd in the interpreter), but an improvement to just
File "gen.pyx", line 9460, in sage.libs.pari.gen._pari_trap (sage/libs/pari/gen.c:45047) PariError: (5)
and trivial enough s.t. it could be merged without any risk. ;-)
(PARI's messages of course aren't always that informative without examining the stack.)
comment:17 Changed 9 years ago by
- Status changed from new to needs_review
With 10018_sig_retry.patch
, all long doctests pass on a 32-bit PPC OS X 10.4, now testing on sage.math.washington.edu
(64-bit Intel Linux).
I also tried very hard to break things in PARI using many different combinations of pari.allocatemem(), other PARI functions and CTRL-C and I did not manage :-)
comment:18 Changed 9 years ago by
- Description modified (diff)
- Keywords pari added
comment:19 Changed 9 years ago by
- Status changed from needs_review to needs_work
Probably this will be absorbed by #9678.
comment:20 Changed 9 years ago by
- Description modified (diff)
- Keywords sig_on sig_retry added; _sig_on _sig_retry removed
comment:21 Changed 8 years ago by
I cannot reproduce this bug. Has it been fixed in another ticket?
comment:22 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:23 Changed 6 years ago by
I tried and I also can't reproduce it. The comments mention #9678 which may have fixed it. I suggest figuring out how this bug was fixed before closing it.
comment:24 Changed 6 years ago by
- Status changed from needs_work to needs_info
I can't reproduce it either. I don't really see the point of trying to find out why this bug existed, since #9678 essentially rewrote the whole error-handling code.
What is the status of 10018_pari_sig_retry.patch? Assuming that this bug has indeed disappeared, is the patch still relevant for the general plan to improve the PARI error handling mechanism (cf. #9640, #14894)?
comment:25 Changed 6 years ago by
#9678 changed a lot of the error-handling code, but it doesn't seem to make any changes to Pari. That's why I'm cautious. That said, I notice that both of these patches are by jdemeyer. He probably understands how these changes interact and if he abandoned this it's probably safe to close this.
Also, attachment:10018_pari_sig_retry.patch looks like it's many changing the memory management in Pari, while the tickets you mention look like they're about the interaction between error management in sage and in pari. They seem unrelated.
comment:26 Changed 6 years ago by
I don't have much time to think of this now, but 10018_pari_sig_retry.patch is indeed changing the PARI error handling mechanism. Let's first look at #9640 and then at this patch.
comment:27 Changed 6 years ago by
- Dependencies set to #9640
- Description modified (diff)
- Keywords init_stack added; segfault segmentation fault allocatemem interrupt sig_on sig_retry removed
- Status changed from needs_info to needs_work
- Summary changed from Unhandled SIGSEGV after bnfcertify() to Improve PARI's stack allocation
comment:28 Changed 6 years ago by
- Description modified (diff)
- Priority changed from critical to minor
comment:29 Changed 6 years ago by
- Cc pbruin added
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:30 Changed 6 years ago by
- Description modified (diff)
Changed 6 years ago by
comment:31 Changed 6 years ago by
- Cc pbruin removed
- Reviewers set to Peter Bruin
- Status changed from needs_review to positive_review
This looks good, it passes doctests, and I agree with the deprecation of init_pari_stack()
.
comment:32 Changed 6 years ago by
- Merged in set to sage-5.13.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
This might be a bug with the handling of
allocatemem()
in PARI. The same computation ingp
gives: