Ticket #244 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

Segmentation fault after Image += in python

Reported by: rowen Owned by: rhl
Priority: minor Milestone:
Component: fw Keywords:
Cc: rhl, smm Blocked By:
Blocking: Project: LSST
Version Number: 3456
How to repeat:

python examples/imageAddSegFault.py

Description

The example file examples/imageAddSegFault.py shows a segmentation fault from using Image += in python. It is easy to work around and may be user error so I've put this at low priority. I mostly wanted to be sure it was documented, but if it can be fixed then so much the better.

The code contains comments showing how to avoid the problem.

Change History

comment:1 Changed 11 years ago by rowen

More info: the error seems to come when memory is reclaimed. I'm guessing that the image (which was obtained from a shared pointer using get) is being double-freed. But it only occurs if += is run. Also I tried using image.this.disown() and that did not help.

comment:2 Changed 11 years ago by rowen

  • Version Number changed from 3454 to 3456

comment:3 Changed 11 years ago by ktl

  • Cc rhl, smm added
  • Status changed from new to assigned

This problem occurs because SWIG always makes the returned proxy object from operator+= (Python _iadd_) the owner of its underlying C++ object. As long as operator+= has the standard semantics, the SWIG wrapper should instead copy the ownership of its first (self) argument. We might be able to work around this with a bunch of %pythonprepend and %pythonappend code, but it looks messy.

comment:4 Changed 11 years ago by smm

Here's another direction we could take. As an example, lets say we want to just fix Image<double>. In fwLib.i, replace

%template(ImageD) lsst::fw::Image<double>;

with

%typemap(out) lsst::fw::Image<double> & {
    if (static_cast<void *>($1) == static_cast<void *>(arg1)) {
        $result = obj0;
        Py_INCREF(obj0);
    } else {
        $result = SWIG_NewPointerObj(SWIG_as_voidptr($1), $descriptor, $owner);
    }
}
%template(ImageD) lsst::fw::Image<double>;
%clear lsst::fw::Image<double> &;

The idea is to check whether the first argument to a function returning an Image<double> & is really the same memory as the return value of the function. If so, the existing wrapper PyObject? is returned, otherwise you get a new one.

The main problem is that

  • it makes assumptions about SWIG naming conventions in generated code (arg1/obj0)
  • you can't easily stop the typemap from being applied to functions which e.g. take no arguments, have weird semantics, etc... (the above works because the only methods in Image returning an Image& are the in-place operators).

So... it's a bit hacky. What it also really needs is for the definition of classes that require this kind of fix to be inside the SWIG .i file. Then one can surround methods with an appropriate %typemap/%clear pair and not worry about inadverdent application of the typemap.

comment:5 Changed 11 years ago by rowen

Is returning a reference to self necessary for in-place operators (+= etc)? I'm surprised if so because I don't see how they can be chained. Even if it's standard perhaps we could get away with returning void instead. Image only defines in-place operators so that might suffice.

comment:6 Changed 11 years ago by ktl

It's legal, if uncommon, in C++ (or C) to say a += (b += c).

If we change operator+= to return void, there will still be an issue with the input argument being disowned by the SWIG wrapper. This could lead to memory leaks.

Instead of putting definitions in the .i file, does it make more sense for us to put #ifdef SWIG code into the .h file?

Perhaps the best solution is just to get a bug fix into SWIG.

comment:7 Changed 11 years ago by ktl

  • Owner changed from ktl to rhl
  • Status changed from assigned to new

FYI, the previous SWIG bug that caused the disown/own code to be added appears to be http://sourceforge.net/tracker/index.php?func=detail&aid=935002&group_id=1645&atid=101645 935002. Again, I think the proper solution is for the ownership status of the result object to be copied if it is the same as the argument object, instead of always set to owned. Since RHL has a track record of submitting SWIG bugs, I'm going to reassign this to him.

comment:8 Changed 11 years ago by rhl

  • Status changed from new to assigned

comment:9 Changed 11 years ago by rhl

  • Status changed from assigned to closed
  • Resolution set to fixed
  • reviewstatus changed from notReady to selfReviewed

This is fixed in the new afw Image classes. The C++ operator returns void, and I %extended the python to work as expected.

comment:10 Changed 10 years ago by robyn

  • Milestone DC2 Integration deleted

Milestone DC2 Integration deleted

Note: See TracTickets for help on using tickets.