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 jdemeyer)

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).

Apply 10018_pari_init_stack.patch

Attachments (3)

10018_sig_retry.patch (20.7 KB) - added by jdemeyer 9 years ago.
Apply on top of #9898
10018_pari_sig_retry.patch (14.9 KB) - added by jdemeyer 9 years ago.
New patch, ignore previous
10018_pari_init_stack.patch (18.0 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Keywords allocatemem added

This might be a bug with the handling of allocatemem() in PARI. The same computation in gp gives:

bash$ sage -gp --fast --stacksize=10000000
                                      GP/PARI CALCULATOR Version 2.4.3 (development svn-12577:12605)
                                       amd64 running linux (x86-64/GMP-4.2.1 kernel) 64-bit version
                                     compiled: Sep 24 2010, gcc-4.6.0 20100705 (experimental) (GCC)
                                              (readline v6.0 enabled, extended help enabled)

                                                  Copyright (C) 2000-2008 The PARI Group

PARI/GP is free software, covered by the GNU General Public License, and comes WITHOUT ANY WARRANTY WHATSOEVER.

Type ? for help, \q to quit.
Type ?12 for how to get moral (and possibly technical) support.

parisize = 10000000, primelimit = 500509
? bnf = bnfinit(polcyclo(23));
? bnfcertify(bnf)
  ***   at top-level: bnfcertify(bnf)
  ***                 ^---------------
  *** bnfcertify: the PARI stack overflows !
  current stack size: 10000000 (9.537 Mbytes)
  [hint] you can increase GP stack with allocatemem()

  ***   Break loop: type 'break' to go back to GP
break>

comment:3 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 9 years ago by leif

  • Cc leif added

comment:5 Changed 9 years ago by leif

  • Description modified (diff)

comment:6 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Keywords interrupt _sig_on _sig_retry added; bnfcertify removed

comment:7 follow-up: Changed 9 years ago by leif

A good place to also add some more case distinctions on PARI's errnum... ;-)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by 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).

comment:9 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Owner changed from tba to jdemeyer

Changed 9 years ago by jdemeyer

Apply on top of #9898

comment:10 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by leif

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 leif

... because of #9640 "not-so-near future".

comment:12 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by jdemeyer

Replying to leif:

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.).

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: Changed 9 years ago by leif

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 jdemeyer

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 leif

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 leif

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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)
  • Keywords pari added

comment:19 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Probably this will be absorbed by #9678.

comment:20 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Keywords sig_on sig_retry added; _sig_on _sig_retry removed

Changed 9 years ago by jdemeyer

New patch, ignore previous

comment:21 Changed 8 years ago by johanbosman

I cannot reproduce this bug. Has it been fixed in another ticket?

comment:22 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:23 Changed 6 years ago by itaibn

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 pbruin

  • 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 itaibn

#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 jdemeyer

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 jdemeyer

  • 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 jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Priority changed from critical to minor

comment:29 Changed 6 years ago by jdemeyer

  • Cc pbruin added
  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:30 Changed 6 years ago by jdemeyer

  • Description modified (diff)

Changed 6 years ago by jdemeyer

comment:31 Changed 6 years ago by pbruin

  • 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 jdemeyer

  • Merged in set to sage-5.13.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.