Opened 11 years ago

Last modified 9 years ago

#11868 closed defect

PARI library interface broken by design — at Version 13

Reported by: Jeroen Demeyer Owned by: William Stein
Priority: major Milestone: sage-5.13
Component: interfaces Keywords: t0GEN t1GEN gen
Cc: Merged in:
Authors: Peter Bruin Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Peter Bruin)

The t0GEN(), t1GEN(),... system in the PARI interface is broken by design. Apply demonstrate_11868.patch to see the bug in action.

The problem is that the conversion of b (Python object) to t1 (PARI GEN) uses the PARI stack and therefore it clobbers the previously computed t0.

There is no urgent need to fix this, as the bug doesn't seem to occur in practice (it was discovered when working on #9334). It only occurs when:

  1. Calling a PARI function with at least 2 arguments besides self.
  2. Apart from the first non-self argument, there should be another argument which is a "complicated" Python type (e.g. a number field element).

As work-around, it is easy to defeat condition 2 by converting arguments to pari before calling the function.

This problem can be solved by using only local variables instead of t0GEN etc.

Apply: trac_11868-t0GEN.patch, trac_11868-remove_workarounds.patch

Change History (16)

comment:1 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

The t0GEN(), t1GEN(),... system in the PARI interface is broken by design. Apply demonstrate_11868.patch to see the bug in action.

Changed 11 years ago by Jeroen Demeyer

Attachment: demonstrate_11868.patch added

Patch to *demonstrate* the bug

comment:2 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.3

Milestone sage-4.7.3 deleted

Changed 9 years ago by Peter Bruin

Attachment: trac_11868-t0GEN.patch added

eliminate global GEN variables

comment:4 Changed 9 years ago by Peter Bruin

Authors: Peter Bruin
Description: modified (diff)
Milestone: sage-5.13
Status: newneeds_review

comment:5 Changed 9 years ago by Peter Bruin

The patch contains (slightly) more changes than strictly necessary; this is to make life a bit easier for #15185.

comment:6 Changed 9 years ago by Jeroen Demeyer

Is it not possible to keep get_nf() as a function returning a GEN without copying?

comment:7 Changed 9 years ago by Jeroen Demeyer

I agree with the general approach, but have to check the many details...

comment:8 Changed 9 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer
Status: needs_reviewneeds_work

In the PARI sources, there are various workarounds for #11868, could you undo these workarounds:

devel/sage/sage/schemes/elliptic_curves/ell_point.py:        We need to explicitly call ``pari()`` because of :trac:`11868`::
devel/sage/sage/rings/number_field/number_field.py:            # to work around Trac #11868 -- Jeroen Demeyer
devel/sage/sage/rings/number_field/number_field.py:        # to work around Trac #11868 -- Jeroen Demeyer
devel/sage/sage/libs/pari/gen.pyx:            sage: pari(K).nfhilbert(t, t+2)  # not tested, known bug #11868

comment:9 Changed 9 years ago by Jeroen Demeyer

Maybe rename get_nf() as _get_nf() to emphazise that it's a private internal function?

comment:10 Changed 9 years ago by Jeroen Demeyer

One important "detail": it seems this ticket causes a lot more copying of data (because of the gcopy() in cdef GEN toGEN. That's bad.

comment:11 Changed 9 years ago by Jeroen Demeyer

Maybe the right thing to do is to have things like t0 of type gen instead of GEN and then using t0.g (like we already often do for self).

comment:12 in reply to:  8 Changed 9 years ago by Peter Bruin

Replying to jdemeyer:

In the PARI sources, there are various workarounds for #11868, could you undo these workarounds:

Done, patch on its way.

Changed 9 years ago by Peter Bruin

remove workarounds for bug related to global GEN variables

comment:13 Changed 9 years ago by Peter Bruin

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