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

Hi Jake,

Thanks for the info. I have spent a bit of time trying to understand how 
that typemap with "reference_container_owner" fragment and swig::traits 
works and have tried to implement a referenced counted class member 
return system with it. Now I may be being stupid here but what I see is:

1. The %typemap ret or out is used in all cases when a value/object is 
"returned" from C++ to Python. So it is used not only when an objects 
member is returned by reference/pointer but also when a C++ function 
returns a value/object by reference/pointer. To handle the case of 
incrementing the reference of a container object only when one of its 
data members is returned something like a %typemap memberout and/or 
memberret needs to be implemented so that it is only used when returning 
data members by reference/pointer.

2. The system uses the "traits" system to, at runtime, determine if the 
return is a reference or not for all functions returning that type.

3. The system adds a pointer to the container within the Python object 
using Python functionality. This appears to be quite 
cumbersome/inefficient to me for such a basic need (reference counting 
of used objects).


For information I have tidied up my small patch a little so that it is 
enabled with %feature("containerReference") either for the whole library 
or on a class by class basis. This appears to work fine although it 
could have issues.

1. This uses the swig code to determine when a reference/pointer to an 
objects member is being returned so handles the requirement of only 
being used when members of objects are being returned. So none of the 
complex traits system is needed, it isn't called unless a member is 
being returned by reference/pointer.

2. A C++ pointer to the container class is added to the members returned 
SwigPyObject and the container classes reference counter is efficiently 
incremented. When a SwigPyObject is destroyed it checks the container 
classes reference counter and if set decrements the containers reference 
counter. The container reference pointer and reference counting is all 
implemented in C++ and is simple, small and efficient.

Unless I am missing something it seems to me:
1. For most languages there needs to be a reference count type of system 
to manage the lifetime of a libraries C++ objects as most of the 
languages that SWIG produces an API for use dynamically 
created/destroyed objects and references to these with garbage 
collectors. Doing this reference counting/lifetime control within the 
C++ SWIG area is simpler and much more efficient.

2. A SWIG internal reference counted container implementation could be 
used by %typemaps such as your use for std::vector simplifying the 
typemap (no fragments etc.) and making this much simpler and more 
efficient. "%typemap(ret) value_type& { SwigContainerReference($self, 
$result); }"

3. The use of %feature("containerReference") for a complete library 
would handle most object member access cases with %typemaps for list 
item returns using the same underlying reference count system. The SWIG 
interface file is simple for this and it would handle all reasonably 
well behaved classes/structs automatically.

As I have said I may be missing something, (possibly fundamental!), but 
this appears to be a much simpler/better way of handling the issue of 
pointers/references to C++ object members ending up pointing to random 
memory contents and indeed, it seems to me, would work better with 
returning items from lists etc.


Terry
On 12/08/2020 03:36, Jake wrote:
> Typemaps are documented here: 
> http://www.swig.org/Doc4.0/SWIGDocumentation.html#Typemaps
>
> This uses the "ret" typemap to add onto whatever the "out" typemap 
> does: http://www.swig.org/Doc4.0/SWIGDocumentation.html#Typemaps_ret
>
> Fragments are a way of reusing a single definition of common code so 
> it doesn't get generated again and again: 
> http://www.swig.org/Doc4.0/SWIGDocumentation.html#Typemaps_fragments
>
> The $-variables are special variables that get substituted with 
> concrete variables or types in the generated code: 
> http://www.swig.org/Doc4.0/SWIGDocumentation.html#Typemaps_special_variables
>
> The type traits stuff is some internal SWIG machinery that I don't 
> think is documented.  Honestly I'm not sure what defines them but they 
> seem to always be defined for types used for container elements.  
> swig::pointer_category indicates it will be a wrapped pointer instead 
> of a copied value.
>
> A puzzle for matching reference return in general is detecting if it 
> belongs to a parent object and isn't e.g. a free function returning a 
> reference.
>
> Last note is that it looks like the code that landed in the released 
> SWIG called it back_reference where I just wrote reference.
>
> -Jake
>
> On Fri, Aug 7, 2020, 6:54 AM Terry Barnaby <[email protected] 
> <mailto:[email protected]>> wrote:
>
>     Hi Jake,
>
>     Thanks for the information. That is one complex typemap syntax !
>     Is there any information on how this works and how to use it ?
>     I will have a try using it to implement a reference count system
>     on all references to members returned from all C++ objects.
>
>     Terry
>     On 04/08/2020 22:35, Jake wrote:
>>     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]
>>     <mailto:[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
>>

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

diff --git a/Lib/python/pyrun.swg b/Lib/python/pyrun.swg
index 63ff82ff8..3635ed846 100644
--- a/Lib/python/pyrun.swg
+++ b/Lib/python/pyrun.swg
@@ -330,8 +330,34 @@ typedef struct {
 #ifdef SWIGPYTHON_BUILTIN
   PyObject *dict;
 #endif
+  PyObject *objContainer;
 } SwigPyObject;
 
+// This is rough experimental code to increase the reference count of the PyObject that wraps a C++ object when a pointer
+// to one of the C++ object's data members is returned in a PyObject. It makes sure the parent C++ object remains in
+// existance whilst a pointer to one of its members is in use.
+// The associated code in cwrap.c and python.cxx assumes all members are returned as a pointer, which is incorrect.
+SWIGRUNTIME SwigPyObject *SWIG_Python_GetSwigThis(PyObject *pyobj);
+
+// This is used when a PyObject pointing ot a C++ objects member is returned
+void SwigContainerReference(PyObject* container, PyObject* member){
+	SwigPyObject*	m = SWIG_Python_GetSwigThis(member);
+	
+	// printf("SwigContainerReference: %p %p %p\n", container, member, m);
+	if(m){
+		Py_INCREF(container);
+		m->objContainer = container;
+	}
+}
+
+// This is called when a PyObject wrapping a C++ object is destroyed
+void SwigContainerDereference(SwigPyObject* obj){
+	//printf("SwigContainerDereference: %p\n", obj);
+	if(obj && obj->objContainer){
+		//printf("SwigContainerDereference: objContainer: %p\n", obj->objContainer);
+		Py_DECREF(obj->objContainer);
+	}
+}
 
 #ifdef SWIGPYTHON_BUILTIN
 
@@ -524,6 +550,9 @@ SwigPyObject_dealloc(PyObject *v)
   } 
   Py_XDECREF(next);
   PyObject_DEL(v);
+
+  // If this object is a reference to a C++ container object, decrement the reference count on the container object
+  SwigContainerDereference(sobj);
 }
 
 SWIGRUNTIME PyObject* 
@@ -740,6 +769,7 @@ SwigPyObject_New(void *ptr, swig_type_info *ty, int own)
     sobj->ty   = ty;
     sobj->own  = own;
     sobj->next = 0;
+    sobj->objContainer = 0;	// Pointer to container object that this object is a member of
   }
   return (PyObject *)sobj;
 }
diff --git a/Source/Modules/python.cxx b/Source/Modules/python.cxx
index c8c45df35..db951970c 100644
--- a/Source/Modules/python.cxx
+++ b/Source/Modules/python.cxx
@@ -3144,6 +3144,10 @@ public:
 #else
       Printf(f->code, "%s\n", tm);
 #endif
+
+      if(GetFlag(parent,"feature:containerReference") && Getattr(n, "memberget"))
+          Printf(f->code, "SwigContainerReference(args, resultobj); // Increment reference count of the container object and add a pointer to the container to this in the returned object\n");
+
       Delete(tm);
     } else {
       Swig_warning(WARN_TYPEMAP_OUT_UNDEF, input_file, line_number, "Unable to use return type %s in function %s.\n", SwigType_str(d, 0), name);