Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong argument in call of tp_new in SWIG_Python_NewShadowInstance for Python3 #1321

Closed
tkrasnukha opened this issue Aug 31, 2018 · 10 comments
Closed

Comments

@tkrasnukha
Copy link

Since Python 3.7 tp_new parameter args must be a tuple (due to PyTuple_GET_SIZE performs assert(PyTuple_Check(args)); now).

Few years ago Andreas Gäer had fixed this issue in his patch.

@wsfulton
Copy link
Member

wsfulton commented Sep 1, 2018

I think this makes sense. I'd like a way to replicate this though to commit with the fix. Could you provide a way to reproduce the assert?

@tkrasnukha
Copy link
Author

I think every call SWIG_Python_NewShadowInstance(clientdata, robj), where clientdata->newraw == nullptr will end up with this assertion.

An example from LLDB project:

resultobj = SWIG_NewPointerObj(new lldb::SBDebugger(
    static_cast<const lldb::SBDebugger&>(result)), SWIGTYPE_p_lldb__SBDebugger, SWIG_POINTER_OWN |  0 );

@wsfulton
Copy link
Member

wsfulton commented Sep 1, 2018

Mmm, I can't replicate this and I'm not convinced the patch is the best solution, it might be hiding a deeper problem. Can you give lots more info? Also which version of SWIG are you using? What the generated SBDebugger python class looks like will be helpful and exactly which lines of code in SwigPyClientData_New are called for this class will give some more clues. What is in clientdata->newargs will also help.

@tkrasnukha
Copy link
Author

tkrasnukha commented Sep 2, 2018

SWIG Version 3.0.12

SwigPyClientData_New executes these lines:

    SwigPyClientData *data = (SwigPyClientData *)malloc(sizeof(SwigPyClientData));
    /* the klass element */
    data->klass = obj;
    Py_INCREF(data->klass);
    /* the newraw method and newargs arguments used to create a new raw instance */
    if (PyClass_Check(obj)) {
      data->newraw = 0;
      data->newargs = obj;
      Py_INCREF(obj);
    }

clientdata->pytype == NULL
SWIG_POINTER_NOSHADOW is not set in flags passed to SWIG_Python_NewPointerObj

Then, in SWIG_Python_NewShadowInstance, in line

inst = ((PyTypeObject*)data->newargs)->tp_new((PyTypeObject*)data->newargs, Py_None, Py_None);

'((PyTypeObject*)data->newargs)->tp_new' points to object_new from typeobject.c from the python.dll.
But since release 3.7, object_new fails with the assertion if the second parameter is not a tuple.

Generated python:

class SBDebugger(_object):
    __swig_setmethods__ = {}
    __setattr__ = lambda self, name, value: _swig_setattr(self, SBDebugger, name, value)
    __swig_getmethods__ = {}
    __getattr__ = lambda self, name: _swig_getattr(self, SBDebugger, name)
    __repr__ = _swig_repr

    def __iter__(self): return lldb_iter(self, 'GetNumTargets', 'GetTargetAtIndex')
    def __len__(self): return self.GetNumTargets()
    def __init__(self, *args):
        """
        __init__(lldb::SBDebugger self) -> SBDebugger
        __init__(lldb::SBDebugger self, SBDebugger rhs) -> SBDebugger
        """
        this = _lldb.new_SBDebugger(*args)
        try:
            self.this.append(this)
        except __builtin__.Exception:
            self.this = this
    __swig_destroy__ = _lldb.delete_SBDebugger
    __del__ = lambda self: None

    # A lot of SBDebugger's methods
    # ...

SBDebugger_swigregister = _lldb.SBDebugger_swigregister
SBDebugger_swigregister(SBDebugger)

@wsfulton
Copy link
Member

wsfulton commented Sep 2, 2018

What Python code are you running to cause the assertion? Something like creating an instance: s = SBDugger() ?

Where is the assertion, can you give file and line info? Also as you have an assertion, this implies you are using a debug build of the python interpreter. I don't think assertions are normally turned on in the Python interpreter, so how did you build your interpreter? Are you using some sort of special build? I presume you are using Windows as you mention dlls? Is it the same on Linux? Looking at object_new I can't see any obvious reason why it would change from 3.6 to 3.7, so are you using the Python interpreter built in the same way as 3.6?

@tkrasnukha
Copy link
Author

tkrasnukha commented Sep 3, 2018

Yes, I use python_d on Windows, it is not a special build, just set debugging symbols and binaries option in installer settings.

The change was not in object_new itself, call sequence looks like object_new -> excess_args -> PyTuple_GET_SIZE(args). And the last executes assert(PyTuple_Check) now.

I have older version of python on linux, so cannot reproduce there. But the issue doesn't look OS-dependent.

@wsfulton
Copy link
Member

wsfulton commented Sep 3, 2018

Okay, I can replicate with a debug build of the Python 3.7 interpreter. Patch makes sense to me now and I've applied it (had to make a few small changes though).

@tkrasnukha
Copy link
Author

It's good, thank you!

@tnovotny
Copy link

Is there going to be a new release anytime soon or will I have to build my own SWIG from head to get the fix?

@wsfulton
Copy link
Member

wsfulton commented Nov 6, 2018

I'm aiming to make the SWIG 4.0 release before the year is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants