Open luciansmith opened 1 month ago
wsfulton
commented
1 month ago swig-4.0 came out well before the compiler version you are using. Can you first check that swig-4.3 works as it has many relevant C++11 improvements. If not, please provide a small cutdown testcase of the source you feed into SWIG.
luciansmith
commented
1 month ago Hmm, it looks like I'm wrong about the version of swig--it uses the directory /usr/share/swig4.0/ but swig itself seems to be 4.2
'apt install swig' gets me 4.2, if I want 4.3, is there a binary somewhere outside of apt, or do I need to compile from source?
luciansmith
commented
1 month ago OK, got 4.3.0 to compile from source, ran it, and got exactly the same error condition, though with slightly changed line numbers:
/media/sf_Desktop/roadrunner/build-ubuntu/wrappers/Python/roadrunner/CMakeFiles/roadrunner_python.dir/roadrunnerPYTHON_wrap.cxx: In function ‘PyObject* _wrap_RoadRunner_getIndependentFloatingSpeciesAmountsV(PyObject*, PyObject*)’:
/media/sf_Desktop/roadrunner/build-ubuntu/wrappers/Python/roadrunner/CMakeFiles/roadrunner_python.dir/roadrunnerPYTHON_wrap.cxx:26860:32: error: cannot bind non-const lvalue reference of type ‘std::vector<double, std::allocator<double> >&’ to an rvalue of type ‘std::vector<double, std::allocator<double> >’
26860 | std::vector<double>& vec = result;
| ^~~~~~
/media/sf_Desktop/roadrunner/build-ubuntu/wrappers/Python/roadrunner/CMakeFiles/roadrunner_python.dir/roadrunnerPYTHON_wrap.cxx:3854:3: note: after user-defined conversion: ‘SwigValueWrapper<T>::operator T&&() const [with T = std::vector<double, std::allocator<double> >]’
3854 | operator T&&() const { return std::move(*pointer.ptr); }
| ^~~~~~~~Unfortunately, I don't have the first idea of how to cut down the source code; it's a huge sprawling project with several dependencies that we've been working on for over a decade. Even an expurgated version would still be enormous.
So, here's one of the actual original functions that have the error:
std::vector<double> RoadRunner::getIndependentFloatingSpeciesAmountsV() const {
check_model();
std::vector<double> result(getNumberOfIndependentSpecies(), 0);
//The independent species are at the beginning of the vector
if (result.size()) {
impl->model->getFloatingSpeciesAmounts(result.size(), 0, &result[0]);
}
return result;
}And here's the (full) wrapped version:
SWIGINTERN PyObject *_wrap_RoadRunner_getIndependentFloatingSpeciesAmountsV(PyObject *self, PyObject *args) {
PyObject *resultobj = 0;
rr::RoadRunner *arg1 = (rr::RoadRunner *) 0 ;
void *argp1 = 0 ;
int res1 = 0 ;
PyObject *swig_obj[1] ;
SwigValueWrapper< std::vector< double,std::allocator< double > > > result;
(void)self;
if (!args) SWIG_fail;
swig_obj[0] = args;
res1 = SWIG_ConvertPtr(swig_obj[0], &argp1,SWIGTYPE_p_rr__RoadRunner, 0 | 0 );
if (!SWIG_IsOK(res1)) {
SWIG_exception_fail(SWIG_ArgError(res1), "in method '" "RoadRunner_getIndependentFloatingSpeciesAmountsV" "', argument " "1"" of type '" "rr::RoadRunner const *""'");
}
arg1 = reinterpret_cast< rr::RoadRunner * >(argp1);
{
try {
{
SWIG_PYTHON_THREAD_BEGIN_ALLOW;
result = ((rr::RoadRunner const *)arg1)->getIndependentFloatingSpeciesAmountsV();
SWIG_PYTHON_THREAD_END_ALLOW;
}
} catch (const std::exception& e) {
SWIG_exception(SWIG_RuntimeError, e.what());
}
}
{
size_t len = (&result)->size();
npy_intp dims[1] = {
static_cast<npy_intp>(len)
};
PyObject *array = PyArray_SimpleNew(1, dims, NPY_DOUBLE);
// VERIFY_PYARRAY(array);
if (!array) {
// TODO error handling.
return 0;
}
double *data = (double*)PyArray_DATA((PyArrayObject*)array);
std::vector<double>& vec = result;
memcpy(data, &vec[0], sizeof(double)*len);
resultobj = array;
}
return resultobj;
fail:
return NULL;
}The line:
std::vector<double>& vec = result;is the one that the compiler errors out on.
Here's the generic 'SwigValueWrapper' code:
#ifdef __cplusplus
#include <utility>
/* SwigValueWrapper is described in swig.swg */
template<typename T> class SwigValueWrapper {
struct SwigSmartPointer {
T *ptr;
SwigSmartPointer(T *p) : ptr(p) { }
~SwigSmartPointer() { delete ptr; }
SwigSmartPointer& operator=(SwigSmartPointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; }
void reset(T *p) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = p; }
} pointer;
SwigValueWrapper& operator=(const SwigValueWrapper<T>& rhs);
SwigValueWrapper(const SwigValueWrapper<T>& rhs);
public:
SwigValueWrapper() : pointer(0) { }
SwigValueWrapper& operator=(const T& t) { SwigSmartPointer tmp(new T(t)); pointer = tmp; return *this; }
#if __cplusplus >=201103L
SwigValueWrapper& operator=(T&& t) { SwigSmartPointer tmp(new T(std::move(t))); pointer = tmp; return *this; }
operator T&&() const { return std::move(*pointer.ptr); }
#else
operator T&() const { return *pointer.ptr; }
#endif
T *operator&() const { return pointer.ptr; }
static void reset(SwigValueWrapper& t, T *p) { t.pointer.reset(p); }
};Looking at the changelog, this was changed here:
2022-06-29: wsfulton
#999 #1044 Enhance SWIGTYPE "out" typemaps to use std::move when copying
objects, thereby making use of move semantics when wrapping a function returning
by value if the returned type supports move semantics.
Wrapping functions that return move only types 'by value' now work out the box
without having to provide custom typemaps.
The implementation removed all casts in the "out" typemaps to allow the compiler to
appropriately choose calling a move constructor, where possible, otherwise a copy
constructor. The implementation also required modifying SwigValueWrapper to
change a cast operator from:
SwigValueWrapper::operator T&() const;
to
#if __cplusplus >=201103L
SwigValueWrapper::operator T&&() const;
#else
SwigValueWrapper::operator T&() const;
#endif
This is not backwards compatible for C++11 and later when using the valuewrapper feature
if a cast is explicitly being made in user supplied "out" typemaps. Suggested change
in custom "out" typemaps for C++11 and later code:
1. Try remove the cast altogether to let the compiler use an appropriate implicit cast.
2. Change the cast, for example, from static_cast<X &> to static_cast<X &&>, using the
__cplusplus macro if all versions of C++ need to be supported.
*** POTENTIAL INCOMPATIBILITY ***Which is you! Thank you for replying so quickly to my bug report.
The SwigValueWrapper code source is from swig.swg
If it helps, here's the entire PYTHON_wrap.cxx file: roadrunnerPYTHON_wrap.cxx.txt
Sorry I can't helpfully cut things down; if it proves impossible to decipher what's going on from the above information the only other thing I can think of to do would be to create a new project from scratch.
luciansmith
commented
1 month ago Further update:
sudo sed -i "681,684d;686d" /usr/share/swig4.0/swig.swg' (which removes the '#if __cplusplus >=201103L' bits, and only uses operator T&() const { return *pointer.ptr; }), the project compiles and runs as expected.sed -i "677,680d;682d" Lib/swig.swg'Also, for what it's worth, MSVC doesn't set __cplusplus correctly, so has always interpreted this code as 'operator T&() const { return *pointer.ptr; }'.
I'm not sure what problem this is trying to fix, but at least in my case, it instead causes other problems. It could simply be that the compiler has been further upgraded and the behavior was reverted? The problem appeared on ubuntu 24, and was not present in ubuntu 22.
wsfulton
commented
1 month ago Your example is a bread and butter basic case. It should work if your SWIG installation is good with the following simple interface file:
%module example
%include <std_vector.i>
%template(VectorDouble) std::vector<double>;
%inline %{
#include <vector>
struct RoadRunner {
std::vector<double> getIndependentFloatingSpeciesAmountsV() const {
return {1.0, 2.0};
}
};
%}which runs nicely with a simple Python file displaying the two elements of the vector:
from example import *
vec = RoadRunner().getIndependentFloatingSpeciesAmountsV()
for v in vec:
print(v)Even if you missed out the %include and %template the code compiles fine, so I suspect you have a bad installation or you have something that is not what you posted. I suggest you try looking at the preprocessed output that swig generates (with swig -E) as well as the preprocessed output of the c++ compiler.
You need to provide the input not the output for anybody else to help, so I suggest you cut down your code to a minimally reproducible example. You also need to include important options to swig and your c++ compiler that might be modifying behaviour.
Our automatic builds that use swig for a C++ project started failing to build the Python bindings when we switched to ubuntu 24 instead of ubuntu 22. I tracked it down to the following lines in the generated wrapper cxx file:
In this context, __cplusplus is indeed >= 201103L, but that code causes errors like the following:
(For the complete context, see https://github.com/sys-bio/roadrunner/actions/runs/12919434975/job/36029944064)
If I edit the generated wrapper to remove the #if, #else, #endif leaving just:
everything compiles fine.
The relevant bits of the wrapped code (the bits around line 26799) are: