Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MATLAB Tests CI and fix double free problem in update #6

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Mar 1, 2022

Adding a CI now that MATLAB is available on GitHub Actions to avoid problems such as #5 in the future.

Just adding the tests reproduced the problem experienced by @HosameldinMohamed in #5 (see tests failing in https://github.com/ami-iit/osqp-matlab-cmake-buildsystem/actions/runs/1915933780 with "double free" error), so the used osqp commit has been updated to point to the newest commit from osqp/osqp-matlab#34 .

We also bumped the version to 0.6.2.2 to be ready to do a release once this is merged.

@traversaro traversaro force-pushed the traversaro-patch-2 branch from 1f6fd53 to a6f6ce9 Compare March 1, 2022 11:39
@traversaro traversaro changed the title Add MATLAB Tests CI Add MATLAB Tests CI and fix double free problem in update Mar 1, 2022
@traversaro
Copy link
Contributor Author

@HosameldinMohamed the PR is ready for review.

Copy link

@HosameldinMohamed HosameldinMohamed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great @traversaro! I don't understand yet the patches you're doing in the upstream repo. But I got what you did here, really impressive!

It's cool that the Tests CI executes the tests available upstream!

Thanks!

@traversaro traversaro merged commit c089f1d into main Mar 1, 2022
@traversaro traversaro deleted the traversaro-patch-2 branch March 1, 2022 21:47
@traversaro
Copy link
Contributor Author

I don't understand yet the patches you're doing in the upstream repo

Sorry, then I was not clear in osqp/osqp-matlab#34 . The idea is that matlab has its own allocator that you can use in place of malloc and calloc from the C standard library, that are called mxMalloc and mxCalloc. Whenever you allocate an area of memory with malloc you need to free it with free, and when you allocate an area of memory with mxMalloc you need to free it with mxFree . In osqp, there is a function called c_malloc that is defined at malloc in some cases (see https://github.com/osqp/osqp/blob/0b34f2ef5c5eec314e7945762e1c8167e937afbd/include/glob_opts.h#L69) or mxMalloc in others (see https://github.com/osqp/osqp/blob/0b34f2ef5c5eec314e7945762e1c8167e937afbd/include/glob_opts.h#L29). However, in many cases the areas allocated with c_malloc was freed was mxFree, that is ok as long as c_malloc is actually mxAlloc , but it is not anymore when c_malloc is actually malloc.

@HosameldinMohamed
Copy link

Sorry, then I was not clear in osqp/osqp-matlab#34

No it's because I am not yet familiar with the whole MATLAB-C integration thing.

Thanks, this explanation helps a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants