Crash when passing a unique_ptr by value about javacpp HOT 12 CLOSED

You're going to have the same problem in C++ if you try to create a std::unique_ptr from an object allocated on the stack. That's how you should think about this. Although we can't actually allocate objects on the stack, that's how it behaves, to make the Java syntax as close as possible to C++.

from javacpp.

A C++ equivalent is:

#include "inc.h"
#include <memory>
#include <stdio.h>

int main(int ac, char **av) {
    std::unique_ptr<G> gp = std::make_unique<G>();
    printf("%d\n", gp->i);
    C c(std::move(gp));
    printf("%p\n", gp.get());
    printf("%d\n", c.get());
}

I don't think there is something wrong. Is it ? It prints the correct:

18
(nil)
18

Management of object G is passed to object C and it shouldn't be destroyed as long as object C lives.

For a real world example, see in PyTorch OptimizerParamGroup and the options argument of its constructor.
Currently my attempts to use it from the presets trigger crashes.

from javacpp.

That's not equivalent. If you call std::make_unique with JavaCPP it's going to do the same thing and it will work.

from javacpp.

Can I ? Using similar trick that we did with @Shared annotation on constructors ?

I'm wondering if, in the UniquePtrAdapter, we shouldn't empty the owner field when we perform the move in U&&() casting operator.

from javacpp.

We could make it so that std::make_unique gets called as part of the constructor of the Java object, sure...

from javacpp.

Ok, Adding @UniquePtr @Name("std::make_unique<G>") to constructors of classes that can be instantiated in Java and passed to native function as unique pointer seems mandatory indeed, like for shared pointers.

However, I still think there is a problem of owner field not cleared after a move:

std::unique_ptr<G> x() { return std::make_unique<G>(); }
void y(std::unique_ptr<G> g) {  }

G g = x();
y(g);
g.deallocate();

Triggers a segmentation fault (double free).
I don't think it's normal. The owner field of g should have been cleared at the same time of its address. Don't you think so ?

from javacpp.

The owner field is fine, that's the std::unique_ptr itself. If it still crashes it's something else

from javacpp.

The owner field of UniquePtrAdapter is the managed pointer owner if I understand well.
As shown in the /* XXX */ line above of the JNI code, this field is used to reinitialize the owner field of the Java object after
the method call. So after a move we end up with a Java object with a null address but a not-null owner, which is deallocated
when the Java object is GCed.
Replacing in the adapter:

operator U&&() { return UNIQUE_PTR_NAMESPACE::move(uniquePtr); }

by

operator U&&() { owner = NULL; return UNIQUE_PTR_NAMESPACE::move(uniquePtr); }

seems to fix the problem, but I don't really know if that makes sense.

from javacpp.

Hum, I think that's going to create a memory leak somewhere, but the existing unit tests for UniquePtrAdapter pass, so... Open a pull request if you're sure you want this, it's fine by me

from javacpp.

Oh, no, actually this can't create a memory leak because we can't modify the deallocator anyway, so that's going to get deallocated fine.

from javacpp.

if you're sure you want this, it's fine by me

I'm sure there is a double free problem somewhere, but not sure at all it's the right fix.
Or whether a similar fix would be needed somewhere: probably in the same cast operator overload in MoveAdapter ?

I'll also push a PR for Pytorch to add the proper annotations on constructors of OptimizerOptions and its subclasses, and probably others. Several other presets are potentially affected: arrow, onnxruntime,onnx, tensorflow-lite, tritonserver.

from javacpp.

I'm sure there is a double free problem somewhere, but not sure at all it's the right fix. Or whether a similar fix would be needed somewhere: probably in the same cast operator overload in MoveAdapter ?

Well it looks fine to me. The pointer is dead anyway after that function call so it can't hurt. Same for MoveAdapter, sure, thanks

I'll also push a PR for Pytorch to add the proper annotations on constructors of OptimizerOptions and its subclasses, and probably others. Several other presets are potentially affected: arrow, onnxruntime,onnx, tensorflow-lite, tritonserver.

👍 Don't bother with Arrow though, I'm not maintaining that one

from javacpp.