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

Passing std::vector<Foo>& to a method when Foo does not have a public default constructor #464

Closed
magzot opened this issue Mar 4, 2021 · 11 comments
Labels

Comments

@magzot
Copy link
Contributor

magzot commented Mar 4, 2021

Trying to generate jni wrappers for a c++ library that has the pattern of creating objects and pushing them into a std::vector that is passed in byref to a factory method. And the types do not have default constructors.

This is how it would look in c++

std::vector<Box> boxes;
BoxFactory factory;
factory.CreateBoxes(10, boxes);

And to use the generated java it would look something like this

JniBox.BoxFactory factory = new JniBox.BoxFactory();
JniBox.Box box = new JniBox.Box(); // <<-- This is private in c++
//JniBox.Box box = new JniBox.Box(new Pointer()); //Would this work?
factory.CreateBoxes(10, box);

Problem is that the Box type does not have a public default constructor so does not seem to be possible to call this factory method using the generated java code.

I have attached a full example including c++ and java code.

javacpp_example.zip

@saudet
Copy link
Member

saudet commented Mar 4, 2021

It sounds like we don't need to create a std::vector<Box> from Java, so this shouldn't be a problem. Passing a new Box(null) for the output argument should indeed just work. It doesn't?

@magzot
Copy link
Contributor Author

magzot commented Mar 4, 2021

ah, you're right :) that just works! wasn't really sure how to use the Pointer types. But, it does not work with the real scenario... though, so guess something else is going wrong there. What's happening is that the "limit" field of the Pointer does now grow. So seems like nothing is added to the vector, or the VectorAdapter does not pick it up some how. But maybe I should compare the generated c++ code for this example and the real scenario and see if I can understand what is going wrong.

I guess you can close this issue then if you like :)

@saudet
Copy link
Member

saudet commented Mar 4, 2021

Well, let me know if you can update it to reflect the real scenario. Thanks!

@magzot
Copy link
Contributor Author

magzot commented Mar 7, 2021

Hi! I have modified my example now so that the generated jni methods in the example and the real lib now look the same (except for names). Also I added an operator=() to the Box class and i think that maybe I found something. Seems like the operator=() is called on an object that has been deleted or not initialized. Is that possible? At least the constructors are not called before = is called. When I did not have the operator=() the copy constructor was called instead. With the real lib the assignment operator is doing things that most often is causing an immediate segfault when called on a deleted object.

Attached new example.

javacpp_example.zip

@saudet
Copy link
Member

saudet commented Mar 8, 2021

std::vector is able to allocate memory without calling the constructor for cases like these, so I'm guessing this is happening in the call to push_back(). Can you confirm this?

@magzot
Copy link
Contributor Author

magzot commented Mar 8, 2021

Hi, added some more logging. It does not seem to be happening in the call to push_back(), but immediately after the call to factory.CreateBoxes(...) returns.

Could it be somewhere in the call to JavaCPP_initPointer?

    ::Box* rptr2 = adapter2;
    jlong rsize2 = (jlong)adapter2.size;
    void* rowner2 = adapter2.owner;
    if (rptr2 != ptr2) {
        JavaCPP_initPointer(env, arg2, rptr2, rsize2, rowner2, &VectorAdapter< ::Box >::deallocate);
    } else {
        env->SetLongField(arg2, JavaCPP_limitFID, rsize2 + position2);
    }

After some reading I get the feeling that calling the copy assignment operator Box& Box::operator=(const Box& rhs) on an uninitialized this is not OK.

@magzot
Copy link
Contributor Author

magzot commented Mar 8, 2021

Or actually it might happen in the type conversion operator operator P*() of the VectorAdapter ? The first line in the snippet above ::Box* rptr2 = adapter2.

@magzot
Copy link
Contributor Author

magzot commented Mar 8, 2021

The generated code of VectorAdapter::operator P*() looks something like this:

Box* p = (Box*)(operator new(sizeof(Box) * boxes.size(), std::nothrow_t()));
std::copy(boxes.begin(), boxes.end(), p);

And with this code I can reproduce the issue now without running javacpp. So I think the issue could be that memory is allocated but not initialized, then copy is called and it is using the copy assignment operator=(). So I guess one needs to understand if this is OK to do, and if it is then there is something wrong with the implementation of operator=() in the lib I am trying to call.

@magzot
Copy link
Contributor Author

magzot commented Mar 8, 2021

Hi, again! :) In my case I think it would be safer to use an in place copy constructor instead:

  Box* p = (Box*)(operator new(sizeof(Box) * boxes.size(), std::nothrow_t()));
  for (int i=0; i<boxes.size(); i++) {
    new (&p[i]) Box(boxes[i]);
  }

That will avoid the operator=(). But maybe this is not general enough.

@magzot
Copy link
Contributor Author

magzot commented Mar 8, 2021

@saudet Hi, this might actually solve the problem?

Box* p = (Box*)(operator new(sizeof(Box) * boxes.size(), std::nothrow_t()));
std::uninitialized_copy(boxes.begin(), boxes.end(), p);

When I run this it is using the copy constructor instead of the assignment operator.

@saudet
Copy link
Member

saudet commented May 6, 2021

Thanks again for the fix! It has been released with JavaCPP 1.5.5.

@saudet saudet closed this as completed May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants