Opened 11 years ago

Last modified 9 years ago

#11868 closed defect

PARI library interface broken by design — at Version 25

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: #864, #9640, #10018 Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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_new.patch, trac_11868-remove_workarounds.patch, 11868_pari_extra.patch

Change History (30)

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)

comment:14 in reply to:  6 Changed 9 years ago by Peter Bruin

Replying to jdemeyer:

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

It is actually even possible that we don't need get_nf() at all, since PARI has higher-level functions like member_pol(), member_diff() etc. that accept various types of number fields. (Maybe these didn't exist when gen.pyx was written?) I'll look into it.

Changed 9 years ago by Peter Bruin

Attachment: trac_11868-get_nf.patch added

eliminate the gen.get_nf() method

comment:15 Changed 9 years ago by Peter Bruin

Description: modified (diff)

It turns out that get_nf() can indeed be done away with entirely.

comment:16 in reply to:  10 ; Changed 9 years ago by Peter Bruin

Replying to jdemeyer:

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.

In an earlier attempt I did make the t0 variables of type gen. I think the reason I changed them back to GEN is to decrease the number of gen objects that had to be created/deleted. The (only) price to pay for this is an extra gcopy() for Sage objects that are converted via their _pari_() method. This is an extremely common case of course. I'll have to think some more about this issue.

comment:17 in reply to:  16 ; Changed 9 years ago by Jeroen Demeyer

Replying to pbruin:

Replying to jdemeyer:

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.

In an earlier attempt I did make the t0 variables of type gen. I think the reason I changed them back to GEN is to decrease the number of gen objects that had to be created/deleted. The (only) price to pay for this is an extra gcopy() for Sage objects that are converted via their _pari_() method. This is an extremely common case of course. I'll have to think some more about this issue.

I don't my suggestion would create more gen objects. What usually happens is (with this patch applied):

  • some PARI computation creates a GEN on the PARI stack
  • _new_gen() calls deepcopy_to_python_heap which copies the object to the Python heap and makes it into a gen object
  • toGEN copies the object back to the PARI stack

My proposal is essentially removing the last step.

comment:18 Changed 9 years ago by Jeroen Demeyer

I found another problem with your patch: you must not do

pari_catch_sig_on()
cdef GEN t0 = P.toGEN(x)

because you should not call Python code within sig_on()/sig_off() and toGEN() potentially calls Python code. So the toGEN should be before pari_catch_sig_on().

comment:19 Changed 9 years ago by Jeroen Demeyer

I have also been thinking about using a with statement:

cdef GEN r
with to_GEN(s) as t0:
    pari_catch_sig_on
    r = gwhatever(t0)
    pari_catch_sig_off
return new_gen(r)  # Clears stack but doesn't call pari_catch_sig_off()

Then to_GEN would be something like

cimport cython

@cython.final
cdef class to_GEN:
    cdef gen x

    def __init__(to_GEN self, s):
        self.x = s._pari_()

    cdef inline GEN __enter__(to_GEN self):
        return self.x.g

    cdef inline int __exit__(to_GEN self, typ, value, traceback):
        return 0

This looks more complicated, but it has the added advantage that the gen object for s is hidden inside the to_GEN class and that the gen x is kept alive for just the right amount of time. This opens a possibility (not on this ticket) to make deepcopy_to_python_heap() lazy, such that it only activates when the PARI stack is cleared.

comment:20 in reply to:  18 Changed 9 years ago by Peter Bruin

Replying to jdemeyer:

I found another problem with your patch: you must not do

pari_catch_sig_on()
cdef GEN t0 = P.toGEN(x)

because you should not call Python code within sig_on()/sig_off() and toGEN() potentially calls Python code. So the toGEN should be before pari_catch_sig_on().

Yes, I also realised in the meantime that this is a problem. With the current policy of clearing the whole stack at every new_gen(), this means that have to wrap every temporary GEN in a gen if we want it to survive subsequent applications of toGEN(). So I think the best solution is to use gen instead of GEN, as in comment:11. (This is essentially very close to what we currently have.)

comment:21 in reply to:  17 Changed 9 years ago by Peter Bruin

Replying to jdemeyer:

I don't my suggestion would create more gen objects.

It doesn't create any more gen objects than what we have now, but I wanted (and failed for now) to create fewer gen objects than what we have now.

What usually happens is (with this patch applied):

  • some PARI computation creates a GEN on the PARI stack
  • _new_gen() calls deepcopy_to_python_heap which copies the object to the Python heap and makes it into a gen object
  • toGEN copies the object back to the PARI stack

My proposal is essentially removing the last step.

That is what happens for objects with a _pari_() method. For other objects, my idea was to not create any gen object and keep the converted GEN on the PARI stack. Ideally, toGEN() should either leave the temporary GEN in the Python heap (if it comes from an existing gen or from a _pari_() method) or on the PARI stack (if it is converted, withouth an intermediate gen, from a Python object without a _pari_() method).

Anyway, this idea is now defeated for the time being because of comment:18 and comment:20. To implement it, we would need a more fine-grained way of clearing the PARI stack (only clear what you have used, and leave the rest of the stack alone).

Now working on a new patch in the spirit of comment:11.

Changed 9 years ago by Peter Bruin

Attachment: trac_11868-t0GEN_new.patch added

eliminate global GEN variables

comment:22 Changed 9 years ago by Peter Bruin

Description: modified (diff)
Status: needs_workneeds_review

trac_11868-t0GEN_new.patch replaces trac_11868-t0GEN.patch and trac_11868-get_nf.patch

Apply trac_11868-t0GEN_new.patch, trac_11868-remove_workarounds.patch

comment:23 Changed 9 years ago by Jeroen Demeyer

Dependencies: #864

comment:24 Changed 9 years ago by Jeroen Demeyer

Dependencies: #864#864, #9640, #10018

comment:25 Changed 9 years ago by Jeroen Demeyer

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