#13447: Make libsingular multivariate polynomial rings collectable
----------------------------------------------------------------+-----------
Reporter: nbruin |
Owner:
Type: defect |
Status: needs_review
Priority: major |
Milestone: sage-5.4
Component: memleak |
Resolution:
Keywords: | Work
issues:
Report Upstream: None of the above - read trac for reasoning. |
Reviewers: Simon King
Authors: Nils Bruin, Simon King | Merged
in:
Dependencies: #11521 |
Stopgaps:
----------------------------------------------------------------+-----------
Comment (by nbruin):
I'm afraid you (at least theoretically) have recreated the possibility for
our original error to occur. We essentially have:
{{{
SingularFunction.__init__:
if currRingHdl == NULL and currRing!=NULL:
currRingHdl = <create ring handle>
currRingHdl.data.uring = currRing
SingularFunction.__call__:
if currRingHdl.data.uring!= currRing:
singular_ring_delete(currRingHdl.data.uring)
currRingHdl.data.uring = singular_ring_reference(currRing)
}}}
If we init with `currRing == NULL` we may be leaving `currRingHdl ==
NULL`. If a subsequent invocation of call happens after `currRing` has a
new value and `currRingHdl` is not updated (if that would never happen we
wouldn't need this code) we'd be calling
{{{
if NULL.data.uring!= currRing:
singular_ring_delete(NULL.data.uring)
NULL.data.uring = singular_ring_reference(currRing)
}}}
which will segfault (OK, this error is much better than the original).
From what you describe, `rKill` can set `currRingHdl=NULL` as well, in
which case we definitely get an error in the above code. Thus it seems to
me it's theoretically possible that `__call__` happens when
`currRingHdl=NULL` and `currRing!=NULL`. In that case you should recreate
a handle:
{{{
if currRingHdl != NULL:
if currRing != NULL:
if currRingHdl.data.uring != currRing:
if currRingHdl.data.uring != NULL:
singular_ring_delete(currRingHdl.data.uring)
currRingHdl.data.uring = currRing #I don't think this
should up refcounts
else: #currRing == NULL and currRingHdl != NULL
rKill(currRingHdl) #delete the handle as well. You could also
save it for reuse
#I think this looks at the refcount
currRingHdl = NULL
else:
if currRing != NULL:
currRingHdl = enterid("my_awesome_sage_ring", 0, RING_CMD,
&IDROOT, 0) #note the 0
currRingHdl.data.uring = currRing
# NOW the following invariant holds:
assert (currRingHdl == NULL and currRing == NUL) or
currRingHdl.data.uring == currRing
}}}
I think that without extra assumptions you must be prepared to deal with
all those conditions. Also note that you might be creating a new
ringhandle an arbitrary number of times, so knowing whether you leak
becomes important. The sequence I use above is what i think the
libsingular examples that are shipped with singular do.
You can already see here that the use of rKill (which does look at
reference counts) forces us to follow Singular's conventions of what the
`ref` field. At this point we're assuming that if currRingHdl holds the
last reference to a ring, we should probably delete it.
I think the convention is that currRingHdl (if it's relevant) is pointing
to the same ring as currRing, so they are treated as one reference:
`currRingHdl.data.uring = currRing` does not increase a refcount. That
obviously leads to problems when `currRing` diverges from `currRingHdl`.
Either you attach memory management ONLY to `currRingHdl` and trust that
when `currRing` is changed, someone else will still have a reference to
the original value, who can throw away the ring (so, `currRing` is only
changed by straight assignments -- no increfs or decrefs. It's always a
borrowed reference) or you always ensure that `currRingHdl` gets updated
(or at least cleared) when `currRing` changes. My impression is that
refcounting in singular is only an interpreter thing, so that the former
is the model used by Singular itself.
'''Double free implies segfault''': Have you verified this is true with
omAlloc?
'''Refcounting base''': I agree that as long as our created rings and
rings created internally live separate lives, it doesn't matter what
semantics we attach to the `ref` field. I just think it's dangerous to
implicitly assume these rings do live separate lives. They do now, but
who's to guarantee that they do in the future? And even then it may take a
long time for a visible error to occur, which by then might be difficult
to track down. I think we should adjust to the semantics Singular uses in
other places (and document the slightly unusual refcounting semantics of
Singular in our code!) to make the libsingular interface as robust as
possible. You know how extensions/modifications are going to happen:
people will just copy/paste what's already there.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13447#comment:63>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
