Do not delete a borrowed reference to reduction strategies in pbori

[jpflori]

In #12313, it was discovered in a painful way that Sage tries to deallocate twice memory allocated for reduction strategies in pbori.

As this fix is now needed by other tickets like #715, I've put the fix on an independent ticket.

CC: @simon-king-jena @alexanderdreyer

Component: memleak

Keywords: segfault

Author: Jean-Pierre Flori, Simon King

Reviewer: Alexander Dreyer

Merged: sage-5.5.rc1

Issue created by migration from https://trac.sagemath.org/ticket/13746

[jpflori]

Patch from #12313 with only changes concerning trac ticket numbers

[jpflori]

comment:1

Attachment: trac_12313_quit_sage.patch.gz

[simon-king-jena]

comment:2

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

I don't recall who wrote most of the code in the patch - in other words: Who can be reviewer for it?

[jpflori]

comment:3

Replying to @simon-king-jena:

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

No, the two patches were orthogonal, they touched different part of quit_sage.
I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).

I don't recall who wrote most of the code in the patch - in other words: Who can be reviewer for it?

I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.

[jpflori]

comment:4

And so this is exactly the same patch as in #12313, except that:

• I've replace 12313 by 13746 in the commit message
• I've replace 12313 by 12313 and 13746 in some comment in the code redirecting the reader to Sage's trac

[simon-king-jena]

comment:5

Replying to @jpflori:

Replying to @simon-king-jena:

OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?

No, the two patches were orthogonal,

Sorry, you are totally right! What I moved to #13471 came from #12215 or so...

I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).

Indeed!

I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.

Not before running a full test...

[alexanderdreyer]

comment:8

I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)

[simon-king-jena]

comment:9

Replying to @alexanderdreyer:

I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)

My guess is that the error is unrelated (it seems to come from a killed Gap session). To be on the safe side, I've just kicked the patchbot, so that it should now repeat the tests.

[alexanderdreyer]

comment:10

Looks good!

[alexanderdreyer]

Reviewer: Alexander Dreyer

[jdemeyer]

Merged: sage-5.5.rc1