Updated basic_free_stack to check if basic variable was stack allocated before freeing

[anutosh491]

This is a function which frees the basic variable only if it has been allocated. The use case comes from here

[anutosh491]

Once I get a green flag, I'll add a test too

[anutosh491]

Not sure why one of the tests fail

[rikardn]

Can basic_new_stack ever return NULL? or rather put NULL in s->m? If that is the case shouldn't we always have the check in basic_free_stack?

[anutosh491]

Yes that makes sense. So I have been using basic_free_stack but I encountered an issue which made me realize that we just simply try freeing the variable without even making sure if it was ever allocated. Hence I would have just liked to update basic_free_stack but wasn't sure if the original implementation should be changed.
Should I just update the basic_free_stack function ?

[rikardn]

There are pros and cons I guess. One could see it as the responsibility of the caller to make sure that the free function is only called on initialized objects, but it is also not possible to check for this using the C-API. I checked how gmp does for its mpz_clear function and it does the check. I think we should also always do the check. A small extra price when freeing, but this is not done so often in application programs. Any other opinions? @certik or @isuruf?

[anutosh491]

Yeah now that I think through it, I guess we should do the check inside basic_free_stack if that's possible !

[rikardn]

Yes, I agree. I think you should do this change

[anutosh491]

I've made the change, also went through basic_free_heap but as that's using the delete keyword I guess we can skip adding the check there.

[certik]

A small extra price when freeing, but this is not done so often in application programs.

At the very least we should put an assert statement that the pointer is not null.

Whether we should always do the check --- I don't think we should. The stack variable is typically a local variable, which will not be null, even if it is not allocated. So for application programs that are hand written, this check will not actually trigger anyway, if the variable is not allocated, it will be a dangling pointer and it will segfault.

I would create a separate function for this, as you did initially. You can put an assert statement into basic_free_stack, but also put a comment there that this assert will only catch bugs if the user initialized the stack variable to null, otherwise it will be a dangling pointer and this check will not catch it.

We can later decide to just always do it, but for now I would keep it separate. One of the main goals of SymEngine is performance, so we should only do operations that are needed, and allowing a path for not doing an operation if the user knows it is not needed.

[anutosh491]

Sure, I think that sounds reasonable. Let me think through this again !

[rikardn]

I agree with @certik about the NULL being uncertain. What I missed in the comparison with libgmp was the call to mpz_init on the mpz_t stack object which sets the allocation to NULL as a starting point. This I guess is similar to using basic_assign in symengine. I am still missing a piece of the puzzle though. How could you set the internal pointer to NULL so that the modified free function can pick this up? Perhaps you could post an example giving a rationale for the basic_free_stack_if_not_null function?

[rikardn]

The definition of basic in the cwrapper is

typedef basic_struct basic[1];

In C I don't think arrays can be NULL so how can the case where this becomes a nullptr ever arise? Could you provide an example where this happens?

[anutosh491]

Could you provide an example where this happens?

So basically everything arises from the issue on Lpython (lcompilers/lpython#2614) . We are trying to replicate the basic type using the following lines

    int64_t _x;
    void* x;
    _x = 0;
    x = NULL;
    x = (void*) &_x;

And then we can use the basic_new_stack on x something like this ....

void main0()
{
    int64_t _x;
    void* x;
    _x = 0;
    x = NULL;
    x = (void*) &_x;
    basic_new_stack(x);
    symbol_set(x, "x");
    printf("%s\n", basic_str(x));
    basic_free_stack(x);
}

[anutosh491]

So something that doesn't work (results into Segmentation Fault) is the following

void mmrv(void* e, void* _lpython_return_variable)
{
    int64_t _stack0;
    void* stack0;
    if (false) {
        _stack0 = 0;
        stack0 = NULL;
        stack0 = (void*) &_stack0;
        basic_new_stack(stack0);
        basic_const_E(stack0);
        printf("%s\n", basic_str(stack0));
    } else {
        basic_assign(_lpython_return_variable, e);
        basic_free_stack(stack0);
        return;
    }
    basic_free_stack(stack0);
}

[rikardn]

Ok, thanks. The use of void pointers and casting here makes me a bit wary. It seems to me that this situation couldn't arise when using the symengine C-API "correctly". So the basic_free_stack_if_not_null would only be useful if you trick the basic type into being NULL outside of the API. In your nonworking example couldn't you simply do:

    if (stack0 != NULL) {
        basic_free_stack(stack0);
    }

at the end?

My feeling is that this function would add confusion to the symengine API if added.

[certik]

We need to add tests.

We are still not sure if this is the way to go, but if we need it, this PR is how to do it. So let's keep it in Draft mode for now, and we'll finish our work in LPython to see what exactly we need.

Thanks @rikardn for your feedback!