This repository was archived by the owner on May 3, 2019. It is now read-only.
Trying to fix memory management errors #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sebasguts
reviewed
Sep 22, 2018
| PyObject * in_as_string = PyObject_Str( in ); | ||
| const char* in_as_c_string = PyUnicodeToString( in_as_string ).c_str(); | ||
| string s = PyUnicodeToString( in_as_string ); | ||
| const char* in_as_c_string = s.c_str(); |
Owner
There was a problem hiding this comment.
Good catch. s might be prematurely deleted. I looked at the C++-Reference, and indeed, the return value of c_str points to the internal memory of the string object.
| return ret_val; | ||
| } | ||
|
|
||
| PyObject* NmzToPyList( mpq_class in ){ |
Owner
There was a problem hiding this comment.
This is indeed unused, probably a left over from PyNormaliz.
| in.get_fmpq_poly(current); | ||
| vector<mpq_class> output; | ||
| fmpq_poly2vector(output,current); | ||
| fmpq_poly2vector(output,current); // is this the correct length?? |
Owner
There was a problem hiding this comment.
I do not understand the length comment? There is no length given here. The function itself comes from e-antic.
Contributor
Author
There was a problem hiding this comment.
Sorry for being cryptic. Question is, do we want to represent number field elements by list of exactly the degree, or list of at most the degree of the number field (when some highest coefficients are 0).
Owner
There was a problem hiding this comment.
Good question. I personally do not care too much. The current solution is easy though, as the conversion function already exists. One could for example resize the vector using the element output handler function.
Contributor
Author
There was a problem hiding this comment.
I'm handling the current format, of course, in sage, so no change is needed. But I was surprised first.
| NumberFieldCone * cone_ptr = reinterpret_cast<NumberFieldCone*>( PyCapsule_GetPointer( cone, cone_name ) ); | ||
| delete cone_ptr->cone; | ||
| delete cone_ptr->nf; | ||
| //delete cone_ptr->nf; |
Owner
There was a problem hiding this comment.
Are you sure nf should not be deleted with the cone? It gets allocated in the cone constructor, see https://github.com/sebasguts/PyQNormaliz/blob/master/QNormalizModule.cpp#L542
Not deleting this would cause a memory leak.
Contributor
Author
There was a problem hiding this comment.
I am not sure about this, but I am getting "pointer being freed was not allocated" without this patch. But since other things go wrong with the memory management that I have not been able to pin down, this may be a fix for the wrong thing.
Owner
There was a problem hiding this comment.
Could you post the valgrind output, too? I am not able to compile the proper sage branch, and as you said, this does not happen in pure python.
| }else{ | ||
| to_compute = PyList_New( arg_len - 1 ); | ||
| for( int i = 1;i<arg_len;i++){ | ||
| for( int i = 1;i<arg_len;i++){ // Is i the correct index?? -mkoeppe |
Owner
There was a problem hiding this comment.
Yes, I am indeed. args[0] is the cone.
Contributor
Author
There was a problem hiding this comment.
But you are using it also as an index into to_compute, which seems off by one.
Is this tested?
Owner
There was a problem hiding this comment.
You are right. I didn't see it, thank you. This needs to be fixed.
Did you try it with the segfault in sage? Cause I doubt that this causes it, as the segfault is when constructing the cone, not when computing something.
Contributor
Author
There was a problem hiding this comment.
In Sage, I don't actually use NmzCompute at the moment, so this bug can't be the cause.
sebasguts
approved these changes
Sep 22, 2018
Contributor
Author
Contributor
Author
|
The sage ticket should be easier to build now. |
Contributor
Author
|
OK here is a segfault that I can reproduce with pure Python: |
Contributor
Author
|
With valgrind: |
Owner
|
@mkoeppe The code above is a segfault, true, but certainly not the one you were looking for. |
Owner
|
I will try to get a working sage to build. If you have a working build, a backtrace of the segfault would be helpful (by running sage in |
Contributor
Author
|
Thanks for the quick fix! |
Contributor
Author
|
I guess it would be better though if it would just compute over Q, just like QNormaliz does. |
Owner
|
Yeah, but certainly this should be done via GMP, not via e-antic then, I guess. But otherwise this is a sensible thing to do. Anyway, the fix does not really help with the problems in sage, unfortunately... |
Contributor
Author
|
OK it seems that the segfaults have gone away. One thing I noticed is that any exception raised in |
Contributor
Author
|
Could you make a new release for the sage ticket? |
Contributor
Author
|
On the sage side, I now avoid calling PyQNormaliz when all data are
rational. So fixing this does not have any urgency.
On Sat, Sep 22, 2018 at 1:53 PM Sebastian Gutsche ***@***.***> wrote:
Yeah, but certainly this should be done via GMP, not via e-antic then, I
guess. But otherwise this is a sensible thing to do.
Anyway, the fix does not really help with the problems in sage,
unfortunately...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH9WhYx_socaimnqzCQHqB4TG6svC063ks5udqNLgaJpZM4W1FIh>
.
--
Sent from my phone
|
Owner
|
So, I do not really understand this right now. Yours and my patch together made the segfaults go away? Anyway, I released 1.2 |
Contributor
Author
|
Thank you for the release. Ticket https://trac.sagemath.org/ticket/25097 seems to work fine with it. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm getting memory management errors, which seem hard to reproduce outside of Sage.
https://trac.sagemath.org/ticket/25097
Valgrind report is inconclusive.
Reviewing the PyQNormaliz source, I found a few suspicious places, but the segfaults persist.