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

OptimizableGraph deletes Parameters even when G2O_NO_IMPLICIT_OWNERSHIP_OF_OBJECTS=ON #695

Open
badicsalex opened this issue Jun 13, 2023 · 3 comments

Comments

@badicsalex
Copy link

Hi,

I'm writing a Rust binding for g2o, and I've been using G2O_NO_IMPLICIT_OWNERSHIP_OF_OBJECTS, because it fits the Rust ownership model (somewhat) better.

I ran into a strange double free with addParameter, and it was surprising to see that the pointer gets released when OptimizableGraph is destroyed. I understand that the documentation of G2O_NO_IMPLICIT_OWNERSHIP_OF_OBJECTS states

Disables memory management in the graph types, this requires the callers to manager the memory of edges and nodes

but it also seems to affect robust kernels for example, and it was surprising to see that it doesn't affect the destruction of parameters.

Is this intended?

@badicsalex
Copy link
Author

I saw that the pymem branch has a lot of related changes, but if possible I'd like to stick with the stable branch for now.

Or is the shared pointer version considered "stable enough"?

@RainerKuemmerle
Copy link
Owner

Sounds like a bug in that feature flag :(
I am overall not that happy with that feature flag. That's also the reason why I explored using shared_ptr and unique_ptr more.
I was soon about to throw those breaking changes onto the master branch. The python wrapper on that branch also benefits from the changed way to handle the memory allocation.
Would the pymem branch overall be better suited for a rust wrapper?

@badicsalex
Copy link
Author

badicsalex commented Jun 14, 2023

I didn't have any other problems with that flag, and managing resources rust-side is actually pretty easy once the ownership problems [1] are straightened out . It's just RAII semantics after all.

I could work with a shared_ptr-based structure. It's probably going to be harder than pointers: the Rust FFI boundary is still pretty C-centric, and shared_ptr has a surprising ABI. Maybe I even have to hide it behind a pointer, creating a double indirection, which would be silly.
Or maybe I'll just create a differently structured, Rust-friendly API wrapper classes in C++ and wrap that instead.
I'll have to see once it goes mainline.

But please don't let this stop you from merging the changes though, smart pointers are way better for C++ users of the library, who should be an overwhelming majority.


[1]: There are bigger problems with wrapping g2o in Rust. For example, Rust lets you have either a single mutable reference or many immutable references to an object, but edges store a pointers to vertices, and graphs also store mutable pointers to vertices. This fact cannot be accurately represented on Rust side, and in this case I chose to ignore the fact that edges store references. Unfortunately this could lead to a use-after-free, should someone use an edge after deleting a vertex for some reason.

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

No branches or pull requests

2 participants