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

CxxWrap fails to distinguish between vectors of shared pointers to value/const value #405

Open
graeme-a-stewart opened this issue Jan 24, 2024 · 8 comments

Comments

@graeme-a-stewart
Copy link

graeme-a-stewart commented Jan 24, 2024

Hello

I have a project to wrap the high-energy physics library HepMC3, where CxxWrap has an issue when there are two functions, where one takes vector of shared pointers to T and the other a vector of shared pointers to const T.

A minimum reproducer is:

#include "jlcxx/jlcxx.hpp"
#include "jlcxx/stl.hpp"

#include <memory>
#include <vector>

void g(std::vector<std::shared_ptr<int>> v){}
void h(std::vector<std::shared_ptr<const int>> w){}


JLCXX_MODULE define_julia_module(jlcxx::Module& types){
  types.method("g", g);
  types.method("h", h);
}

A library built to wrap these functions will fail to load with the error:

ERROR: LoadError: Double registration for method (:constructor, :StdVector, :dr, 0xa0c245c5f9fa2716)

This doesn't happen with bare pointers, but unfortunately the library I am wrapping makes heavy used of shared_ptr and it's a blocker for us not to have the two versions supported.

Thanks in advance for any fixes or suggestions (and thanks to @grasph for helping track this down).


P.S. The actual library code causing the issue is found in cases like this:

  • std::vector<GenParticlePtr> parents(); defined here
  • std::vector<ConstGenParticlePtr> parents() const; defined here

where GenParticlePtr and ConstGenParticlePtr types are:

using GenParticlePtr = std::shared_ptr<GenParticle>;
using ConstGenParticlePtr = std::shared_ptr<const GenParticle>;

as defined here.

@barche
Copy link
Collaborator

barche commented Jan 25, 2024

I can confirm that this is a bug in CxxWrap, but it's not that easy to fix unfortunately. If there is a class Foo, then the code mapping types between C++ and Julia doesn't create separate types for Foo and const Foo, so that is the root cause of this problem and not very easy to fix. For now the only workaround I can think of is to only wrap the non-const versions.

@graeme-a-stewart
Copy link
Author

Hi Bart - thanks for taking a look and confirming the issue 🙏. When you say "not very easy to fix" does that mean it would require some deep restructuring of CxxWrap? And is that envisaged?

Anyway, we will try the work around for now.

@dannys4
Copy link

dannys4 commented Jan 29, 2024

@graeme-a-stewart Is there an easy-ish way to figure out which methods are causing this? I'm getting hampered by this as well, though I don't mind changing/removing some bindings--I'm just lazy and would prefer to expend minimal effort trying to figure out what went wrong 😅

@graeme-a-stewart
Copy link
Author

Hi @dannys4

The way that we did it (credit to @grasph) is to develop CxxWrap and patch the place where the error is generated to be more informative:

diff --git a/src/CxxWrap.jl b/src/CxxWrap.jl
index 91a056a..47d4835 100644
--- a/src/CxxWrap.jl
+++ b/src/CxxWrap.jl
@@ -358,7 +358,7 @@ function _register_function_pointers(func, precompiling)
   if haskey(__global_method_map, mkey)
     existing = __global_method_map[mkey]
     if existing[3] == precompiling
-      error("Double registration for method $mkey")
+      error("Double registration for method $mkey: $(func.name); $(func.argument_types); $(func.return_type)")
     end
   end
   __global_method_map[mkey] = fptrs

That helps a lot over the standard error message. e.g., I could now see:

ERROR: LoadError: Double registration for method (:constructor, :StdVector, :HepMC3, 0x752d0efde0ec6159): CxxWrap.CxxWrapCore.ConstructorFname(CxxWrap.StdLib.StdVector{CxxWrap.StdLib.SharedPtr{Main.HepMC3.HepMC3!GenParticle}}); Any[]; Any

N.B. I had to specifically checkout CxxWrap v0.14.2, the current head of main isn't working for me (julia(24987,0x1dfbb1000) malloc: Heap corruption detected, free list is damaged at 0x600000694910).

@dannys4
Copy link

dannys4 commented Jan 30, 2024

I ended up coming up with an equivalent (albeit more crude) solution to see that. It turns out my issue was unrelated; I got the same error from linking my JLL against libcxxwrap version 0.12.0 but using cxxwrap in Julia with libcxxwrap version 0.11.2 . Sorry for the hassle and thanks for the help nonetheless!

@graeme-a-stewart
Copy link
Author

At least one debugging route is recorded for posterity! In fact, @barche, would it be useful to patch this in CxxWrap? The current error through is really not very helpful/useful.

@barche
Copy link
Collaborator

barche commented Feb 4, 2024

Indeed, it would be useful to add more verbose error reporting. I am also close to having a fix for the root problem here, which is that right now CxxWrap always uses Foo<X> even if in C++ there is Foo<const X>, which allowed for simpler code, but obviously can (and did here) cause problems.

@barche
Copy link
Collaborator

barche commented Feb 11, 2024

Tests on a fix for this are now passing in JuliaInterop/libcxxwrap-julia#145 and in the CxxWrap testjll branch, so feel free to try it out before I merge this.

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

3 participants