Re: C++ to Python and returning members of a C++ object
Terry Barnaby <[email protected]>
| Newsgroups | gmane.comp.programming.swig |
|---|---|
| Message-ID | <[email protected]> |
Hi William, As stated previously I have some reasonably large object orientated C++ class libraries where classes/structs make use of class members extensively and at depth. To have to add special typemap for all of the cases where member objects are returned in all situations would end up with a lot of work, a lot of SWIG code to handle all the possible cases, would be difficult to test and, I think, unmaintainable. I assume your idea with a typemap memberout would end up being similar in that I would have to add a memberout typemap for every class/struct type in the library ? If so I would have thought this memberout code would end up having to increment the reference on the container object and having some code that decrements the reference on the container object when the returned member is deleted similar to my simpler proposed method. From my perspective, the ideal for SWIG, would be to wrap these libraries with the absolute minimum of SWIG code with it just using the C++ headers. I nearly have this apart from the returning of dangling pointers, handling C fixed arrays (which I think SWIG could also automatically do) and adding Python list methods to simplify the use of the C++ lists in Python. It seems to me that the current implementation (C++ -> Python), where a member of a C++ object is returned by reference without incrementing the reference on the container object is fundamentally flawed. It quickly leads to dangling memory pointers and segment faults that are horrible for a user to track down and the fix of telling the user to create a copy of each C++ member object they want to return from a Python function is also not good. I thought my simple idea of just incrementing the reference of the containing object whenever a pointer to its internal data is returned for use and then decrementing this reference when the returned member goes out of scope was the simplest, easiest and most tidy way of achieving this with no SWIG code (apart from turning the feature on at the top of the SWIG code file). It is only a few lines of code added to SWIG and it should work in all reference memberout cases and all depths automatically with very little overhead. This would likely work for the std:vector and all other such cases automatically. As far as I can see the only detractor is the delayed container object destruction that could affect some code, but I would have thought that would not affect many cases and in these the feature could be turned off for the particular classes or the user asked to make a copy of the returned member. Now I may be missing something fundamental in this, but if not (and no one so far has stated why this won't work), why don't you think this would be a useful feature in SWIG ? Terry On 27/07/2020 22:30, William S Fulton wrote: > Hi Terry > > I don't know why you have not based your solutions on the approaches > mentioned in my original response. One of those is the std::vector > solution that Jake also brought up. What I think might work is, > instead of a new feature, is a new typemap that is only used when > wrapping 'getter' member variables. Let's call it say memberout. It > will contain the typemap code that I posted originally and only be > generated when wrapping member variable getters. I'd need to think how > to make it work only when needed, which I think is for all member > types stored by value that are a SWIG wrapped proxy class type. > > William > > On Wed, 22 Jul 2020 at 13:45, Terry Barnaby <[email protected] > <mailto:[email protected]>> wrote: > > In effect, this is what my experimental patch does at the SWIG C++ > wrapping level. It adds a PyObject* to the members SwigPyObject > that is set to point to the container object that the members > SwigPyObject is referencing. This PyObject* pointer is set on a > members SwigPyObject object when a pointer/reference to a member > is created as well as the container object having its reference > counter incremented. When the members SwigPyObject is deleted it > will use the PyObject* to decrement the container objects > reference counter if the PyObject* is actually set. There is the > addition of the pointer to every SwigPyObject and the call to the > reference counter increment/decrement but that is all so it seems > pretty efficient if this method is ok. > > It is simple, but I am unsure of its implications in the wider > SWIG codebase as I don't have much knowledge of this. So any > comments on this approach welcome. > If ok, I will have to work out how to use the "feature" > functionality of SWIG to implement it properly. > > Terry > > On 22/07/2020 13:00, Jake wrote: >> I support this idea too. I had a PR that got into 4.0 for >> essentially the same issue on containers like std::vector that >> might be of interest: >> >> https://github.com/swig/swig/pull/1234 >> >> Strategy there was to point to the parent wrapper from the child >> wrapper to tie their GC lifetimes together. >> >> -Jake >> >> On Wed, Jul 22, 2020, 7:42 AM Terry Barnaby <[email protected] >> <mailto:[email protected]>> wrote: >> >> Hi, >> >> Yes, it is a very simple patch that just increments the >> reference counter on the SWIG wrapped C++ object when a >> member of it is returned via a pointer/reference and >> decrements that reference when the returned member is finally >> deleted. The idea would be to implement this as a SWIG >> feature than could be turned on for an individual >> class/struct or the whole library. I believe it would be of >> use if quite a few cases, but as said, I may be missing >> something. >> >> Terry >> On 22/07/2020 10:57, D Haley wrote: >>> >>> Dear List, >>> >>> I would like to support the demand side of things for this, >>> I maintain a small library that provide C++->python using >>> swig, and we've had troubles with library users having >>> memory corruption when using python. Our workaround has been >>> to discourage the inadvertent creation of temporary >>> variables, but it seems a poor solution. >>> >>> As a library maintainer, it sounds like (and I've not gone >>> through the patch in detail), that this reference counting >>> approach would help minimise the chance of corruption by >>> early collection? >>> >>> Thanks. >>> >>> On 22/07/2020 10:10, Terry Barnaby wrote: >>>> Hi, >>>> >>>> Anyone any views on this ? >>>> I am happy to try and create a patch to add this as a >>>> "feature" enabled function if people think it would be >>>> worthwhile and would be included in the system. It >>>> certainly seems to be a good idea from our libraries point >>>> of view. >>>> >>>> Terry >>>> On 08/07/2020 06:57, Terry Barnaby wrote: >>>>> Hi William, >>>>> >>>>> So do you think this idea of incrementing the reference >>>>> counter on the C++ object whose member you are returning >>>>> by reference is reasonable and would be a good addition to >>>>> SWIG as a "feature" that can be enabled ? If so I can look >>>>> at producing a proper patch for adding this (have to work >>>>> out how the features system works first). As I don't know >>>>> the SWIG code in depth it would need a good look at once I >>>>> have done it. >>>>> >>>>> Terry >>>>> On 01/07/2020 11:50, Terry Barnaby wrote: >>>>>> Hi William, >>>>>> >>>>>> Enclosed is my very experimental, basic and not for use >>>>>> example of the sort of thing I was thinking of. >>>>>> This is Python specific has quite a few hard coded things >>>>>> that won't work in all cases etc etc and assumes all >>>>>> members are returned as a pointer, which is incorrect. >>>>>> >>>>>> But with a lot more work, maybe something like this could >>>>>> be made to work when enabled using a particular "feature" >>>>>> setting ? >>>>>> >>>>>> Basically what it should do is whenever a pointer to a >>>>>> C++ objects member is returned, it will increment the >>>>>> reference counter on the SWIG wrapper for the container >>>>>> object and set a pointer to this container on the >>>>>> returned member pointer PyObject. Then when the member's >>>>>> pointer PyObject is deleted, it will decrement the >>>>>> reference counter on the overall C++ container object so >>>>>> it can be deleted. >>>>>> >>>>>> I'm probably missing a few things here, but I wonder if >>>>>> this sort of mechanism could handle a lot of normal >>>>>> situations when pointers to C++ members are returned ? >>>>>> >>>>>> Terry >>>>>> On 01/07/2020 06:44, Terry Barnaby wrote: >>>>>>> Hi William, >>>>>>> >>>>>>> Thanks for the information. In the case of the class >>>>>>> library I'm wrapping that would end up with an awful >>>>>>> amount of code in SWIG to handle every case where this >>>>>>> occurs. Most, if not all of the classes/structs I am >>>>>>> encapsulating use other classes/structs as members down >>>>>>> multiple levels and changes would be needed to this code >>>>>>> for every library update. It might end up it would be >>>>>>> easier not to use SWIG :) >>>>>>> >>>>>>> Yes, memory management, pass by value, reference/pointer >>>>>>> etc is tough, but I am surprised that SWIG doesn't >>>>>>> handle this, at least for Python, with a user >>>>>>> configurable "feature" or all of the time. From what I >>>>>>> can see for C++ -> Python at least it would be possible >>>>>>> to add the code, as given in the references you give, by >>>>>>> default whenever a reference to a C++ object members >>>>>>> data is being returned as a pointer. I think it would be >>>>>>> a generally the case that if you return a reference to a >>>>>>> C++ objects member you would want the overall C++ object >>>>>>> to hang around while that pointer is in existence. >>>>>>> >>>>>>> Yes, this would reduce efficiency, but if instead of the >>>>>>> methods suggested a simpler and more efficient >>>>>>> Py_INCREF() and Py_DECREF() mechanism on the PyObject* >>>>>>> to the parent object could be used the overhead should >>>>>>> be pretty small. I have tried a simple implementation of >>>>>>> this by modifying SWIG to do the Py_INCREF() and it >>>>>>> appears to work well (although it is a hack as I don't >>>>>>> understand the SWIG internals well enough) and I don't >>>>>>> know how to call Py_DECREF() efficiently so I am not >>>>>>> doing this. I am assuming it would be possible to modify >>>>>>> the SWIG C++ wrapper object to have the extra container >>>>>>> objects pointer so its reference count can be >>>>>>> decremented but I need to try and understand the SWIG >>>>>>> code better. >>>>>>> >>>>>>> If you think something like this might be feasible and >>>>>>> useful, I will have a look at the SWIG code a bit more >>>>>>> in my limited spare time. >>>>>>> >>>>>>> Terry >>>>>>> On 28/06/2020 13:32, William S Fulton wrote: >>>>>>>> Hi Terry >>>>>>>> >>>>>>>> This is one of the most horrible problem areas mixing >>>>>>>> languages, that is, memory management. Here is some >>>>>>>> background reading: >>>>>>>> >>>>>>>> http://www.swig.org/Doc4.0/Python.html#Python_memory_management_member_variables >>>>>>>> >>>>>>>> https://github.com/swig/swig/issues/945 >>>>>>>> https://github.com/swig/swig/pull/1234 >>>>>>>> >>>>>>>> Below is one way you can fix this (it's a variation of >>>>>>>> returning members by reference documented in the 1st >>>>>>>> link above). Note that SWIG uses pointers for accessing >>>>>>>> non-primitive member variables, see >>>>>>>> http://www.swig.org/Doc4.0/SWIG.html#SWIG_structure_data_members. >>>>>>>> >>>>>>>> >>>>>>>> %fragment("member_parent_attribute_init", "init") { >>>>>>>> // thread safe initialization >>>>>>>> member_parent_attribute(); >>>>>>>> } >>>>>>>> >>>>>>>> %fragment("member_parent_attribute", "header", >>>>>>>> fragment="member_parent_attribute_init") { >>>>>>>> static PyObject* member_parent_attribute() { >>>>>>>> static PyObject* attr = >>>>>>>> SWIG_Python_str_FromChar("__member_parent"); >>>>>>>> return attr; >>>>>>>> } >>>>>>>> } >>>>>>>> %typemap(ret, fragment="member_parent_attribute", >>>>>>>> noblock=1) ListA *TypeB::list1 %{ >>>>>>>> PyObject_SetAttr($result, member_parent_attribute(), >>>>>>>> $self); >>>>>>>> %} >>>>>>>> >>>>>>>> William >>>>>>>> >>>>>>>> On Thu, 25 Jun 2020 at 14:58, Terry Barnaby >>>>>>>> <[email protected] <mailto:[email protected]> >>>>>>>> <mailto:[email protected]> >>>>>>>> <mailto:[email protected]>> wrote: >>>>>>>> >>>>>>>> I have had a quick look at the SWIG code. >>>>>>>> >>>>>>>> It seems that it would be possible, and desirable >>>>>>>> to do a >>>>>>>> Py_INCREF() on >>>>>>>> the PyObject* object that is wrapping the C++ >>>>>>>> object which you are >>>>>>>> returning a pointer to the member from. This would >>>>>>>> hold the >>>>>>>> whole 'C++' >>>>>>>> object in memory so that returning a pointer to one >>>>>>>> of this >>>>>>>> objects >>>>>>>> members would be ok. The PyObject* returned for the >>>>>>>> member >>>>>>>> would have to >>>>>>>> have a pointer to its parents PyObject* and do a >>>>>>>> Py_DECREF() on >>>>>>>> the >>>>>>>> parent object when it is finally deleted so that >>>>>>>> the original >>>>>>>> C++ object >>>>>>>> gets cleaned up eventually. >>>>>>>> >>>>>>>> I don't know the SWIG code to know if this is >>>>>>>> really possible >>>>>>>> and if >>>>>>>> people think this is a reasonable SWIG feature that >>>>>>>> could be >>>>>>>> added ? >>>>>>>> At the moment a user of your SWIG generated Python >>>>>>>> API gets a >>>>>>>> pointer to >>>>>>>> a destroyed object which is not good. >>>>>>>> >>>>>>>> Terry >>>>>>>> On 24/06/2020 10:04, Terry Barnaby wrote: >>>>>>>> > I have a class library that has List objects as >>>>>>>> members of >>>>>>>> the class. >>>>>>>> > If I use this class in a Python function and try >>>>>>>> to return >>>>>>>> one of >>>>>>>> > these more complex members all hell breaks loose. >>>>>>>> It appears >>>>>>>> the SWIG >>>>>>>> > generated wrapper returns a pointer to the >>>>>>>> objects member >>>>>>>> rather than >>>>>>>> > making a copy of it when returning the member >>>>>>>> from the function. >>>>>>>> > >>>>>>>> > So I have something like: >>>>>>>> > >>>>>>>> > class TypeB { >>>>>>>> > >>>>>>>> > public: >>>>>>>> > >>>>>>>> > List<TypeA> list1; >>>>>>>> > >>>>>>>> > }; >>>>>>>> > >>>>>>>> > If in Python I have a function like the >>>>>>>> following, the >>>>>>>> returned item >>>>>>>> > is effectively (SWIG wrapped) a pointer the b >>>>>>>> objects member >>>>>>>> and b >>>>>>>> > disappears when the function returns. >>>>>>>> > >>>>>>>> > def func1(): >>>>>>>> > >>>>>>>> > b = TypeB(); >>>>>>>> > >>>>>>>> > b.list1.append(TypeA()); >>>>>>>> > >>>>>>>> > return b.list1; >>>>>>>> > >>>>>>>> > Now I can use "return ListTypeA(b.list1)", where >>>>>>>> ListTypeA >>>>>>>> has been >>>>>>>> > declared using a %template declaration such that >>>>>>>> the copy >>>>>>>> constructor >>>>>>>> > is called to explicitly make a copy of the list, >>>>>>>> but is there >>>>>>>> a better >>>>>>>> > way of handing this ? >>>>>>>> > >>>>>>>> > I know this is difficult to handle, but perhaps >>>>>>>> something like a >>>>>>>> > %template(return) to add code when returning a >>>>>>>> type in a Python >>>>>>>> > function somehow or maybe the overall objects >>>>>>>> reference >>>>>>>> counter can be >>>>>>>> > incremented ? >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> > _______________________________________________ >>>>>>>> > Swig-user mailing list >>>>>>>> > [email protected] >>>>>>>> <mailto:[email protected]> >>>>>>>> <mailto:[email protected]> >>>>>>>> <mailto:[email protected]> >>>>>>>> > >>>>>>>> https://lists.sourceforge.net/lists/listinfo/swig-user >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Swig-user mailing list >>>>>>>> [email protected] >>>>>>>> <mailto:[email protected]> >>>>>>>> <mailto:[email protected]> >>>>>>>> <mailto:[email protected]> >>>>>>>> https://lists.sourceforge.net/lists/listinfo/swig-user >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> Swig-user mailing list >>>> [email protected] <mailto:[email protected]> >>>> https://lists.sourceforge.net/lists/listinfo/swig-user >>> >>> >>> _______________________________________________ >>> Swig-user mailing list >>> [email protected] <mailto:[email protected]> >>> https://lists.sourceforge.net/lists/listinfo/swig-user >> >> >> _______________________________________________ >> Swig-user mailing list >> [email protected] >> <mailto:[email protected]> >> https://lists.sourceforge.net/lists/listinfo/swig-user >> > > _______________________________________________ > Swig-user mailing list > [email protected] > <mailto:[email protected]> > https://lists.sourceforge.net/lists/listinfo/swig-user > _______________________________________________ Swig-user mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/swig-user