Re: C++ to Python and returning members of a C++ object

I don't believe anyone has said it would not be useful.  There are just
some questions about implementation.

Since the Python container patch is in SWIG already and includes machinery
to set a back-reference (it effectively does the equivalent of
`child.__owner = parent` in Python), and this also accomplishes automatic
reference/dereference, why not re-use it rather than re-implement in a
different way?  It also does not require modifying the native-code part of
SWIG; much of SWIG's language support is written in SWIG's language.

Typemaps can be a little tricky to place sometimes but can be pretty
generic.  You seem to think you'd have to repeat them for all your types,
but this isn't really the case, e.g. my container patch sticks one in:

    %typemap(ret, fragment="reference_container_owner", noblock=1)
value_type& {

(void)swig::container_owner<swig::traits<$*1_ltype>::category>::reference($result,
$self);
    }

This applies to all the container functions returning a reference to the
value type.  There is even a placeholder type `SWIGTYPE` you could use, so
something like

%typemap(out) SWIGTYPE&

Would apply to everything returned by reference, it does not need to be
repeated type-by-type.  However, it does give the user the option of
overriding the behavior by specifying their own typemap at the same or a
more specific level.

The other thing the container patch has that could be re-used is that
template specialization on the type traits, which lets you distinguish
things which are wrapped as pointers vs values.  This is to prevent setting
a back-reference to something that got copied like an int, less overhead
and you don't pin the parent memory unnecessarily.

William probably has some other subtleties in mind on when it should apply
or not.

-Jake

On Tue, Jul 28, 2020 at 1:44 AM Terry Barnaby <[email protected]> wrote:

> 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]> 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]> 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]> <[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]>
>>> <[email protected]>
>>>     > https://lists.sourceforge.net/lists/listinfo/swig-user
>>>
>>>
>>>
>>>
>>>     _______________________________________________
>>>     Swig-user mailing list
>>>     [email protected]
>>>     <mailto:[email protected]>
>>> <[email protected]>
>>>     https://lists.sourceforge.net/lists/listinfo/swig-user
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Swig-user mailing [email protected]://lists.sourceforge.net/lists/listinfo/swig-user
>>>
>>>
>>>
>>> _______________________________________________
>>> Swig-user mailing [email protected]://lists.sourceforge.net/lists/listinfo/swig-user
>>>
>>>
>>> _______________________________________________
>>> Swig-user mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/swig-user
>>>
>>
>> _______________________________________________
>> Swig-user mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/swig-user
>>
>
>

_______________________________________________
Swig-user mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/swig-user