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

Fix RaithData memory leaks #271

Closed
wants to merge 2 commits into from
Closed

Conversation

MatthewMckee4
Copy link

Fix for #270.

Changes:

  • updated type hints so raith_data is optional and its attributes are immutable
  • Made FlexPath.raith_data in c++ optional<RaithData>
  • moved raith to_gds functionality to the RaithData struct
  • (accidentally) reformatted .pyi file (I can change this back if this is an issue)
  • add tests for RaithData
  • update tests for FlexPath.rath_data

@MatthewMckee4
Copy link
Author

@heitzmann do you know what has causes this error? I am developing on Ubuntu 24.04 so was unaware of such an issue with macos and windows

@gnawhleinad
Copy link
Contributor

From macos-latest run in https://github.com/heitzmann/gdstk/actions/runs/10512919194/job/29141203848?pr=271,

/Applications/Xcode_15.4.app/Contents/Developer/usr/bin/g++  -I/Users/runner/work/gdstk/gdstk/include -I/Users/runner/work/gdstk/gdstk/external -I/opt/homebrew/include -Wall -Wextra -Wshadow -Wvla -Wformat -Wno-missing-field-initializers -Wno-missing-braces -Wno-cast-function-type -Wno-unused-parameter -mmacosx-version-min=10.9 -O3 -DNDEBUG -O3 -DNDEBUG -std=c++11 -arch arm64 -isysroot /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk -fPIC -MD -MT src/CMakeFiles/gdstk.dir/flexpath.cpp.o -MF src/CMakeFiles/gdstk.dir/flexpath.cpp.o.d -o src/CMakeFiles/gdstk.dir/flexpath.cpp.o -c /Users/runner/work/gdstk/gdstk/src/flexpath.cpp
In file included from /Users/runner/work/gdstk/gdstk/src/flexpath.cpp:20:
/Users/runner/work/gdstk/gdstk/include/gdstk/flexpath.hpp:78:10: error: no template named 'optional' in namespace 'std'
    std::optional<RaithData> raith_data;
    ~~~~~^
1 error generated.

Or, very specifically, the -std=c++11.

<optional> was added in c++17 via https://en.cppreference.com/w/cpp/header/optional.

I suspect the Windows failure is similar.

@heitzmann
Copy link
Owner

@MatthewMckee4 That's correct, we have to switch to C++ 17 to use optional. From your changes, I believe the leak comes from the copy_from function. I patched it in 69fbde1 just to do a quick release because of other changes. You can try that version to see if it works, or make sure we can compile correctly in C++17 and continue this PR.

@MatthewMckee4
Copy link
Author

in RaithData::copy_from the code is as follows

    ...
    if (base_cell_name) free_allocation(base_cell_name);
    base_cell_name = NULL;
    if (raith_data.base_cell_name) base_cell_name = copy_string(raith_data.base_cell_name, NULL);

I think it should just be

    ...    
    base_cell_name = NULL;
    if (raith_data.base_cell_name) base_cell_name = copy_string(raith_data.base_cell_name, NULL);

When the free_allocation call is there there's a memory leak:

free(): invalid pointer
Aborted (core dumped)

@heitzmann
Copy link
Owner

heitzmann commented Sep 1, 2024 via email

@MatthewMckee4
Copy link
Author

Sorry for not getting back to you sooner, i believe this is fixed so will close this issues

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.

3 participants