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

Possible regression in enum handling with pyside2 #14156

Open
2 tasks done
NomAnor opened this issue May 19, 2024 · 5 comments
Open
2 tasks done

Possible regression in enum handling with pyside2 #14156

NomAnor opened this issue May 19, 2024 · 5 comments
Labels
Bug This issue or PR is related to a bug OS: Linux WB Part Design Related to the Part Design Workbench

Comments

@NomAnor
Copy link
Contributor

NomAnor commented May 19, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Problem description

With the current set of packages on Arch Linux I get a SIGABRT when calling the "Involute Gear" command from PartDesign.

#0  0x00007ffff09c0e44 in ??? () at /usr/lib/libc.so.6
#1  0x00007ffff0968a30 in raise () at /usr/lib/libc.so.6
#2  0x00007ffff09504c3 in abort () at /usr/lib/libc.so.6
#3  0x00007ffff09503df in ??? () at /usr/lib/libc.so.6
#4  0x00007ffff0960c67 in __assert_fail () at /usr/lib/libc.so.6
#5  0x00007ffff0ed6734 in Shiboken::Enum::getValue(_object*) () at /usr/lib/libshiboken2.cpython-312-x86_64-linux-gnu.so.5.15
#6  0x00007ffff6bae0a4 in Gui::PythonWrapper::toEnum (this=0x7fffffff961b, pyPtr=0x7fff3c4d2610) at ../../src/Gui/PythonWrapper.cpp:592
#7  0x00007ffff6bae0d6 in Gui::PythonWrapper::toEnum (this=0x7fffffff961b, pyobject=...) at ../../src/Gui/PythonWrapper.cpp:601
#8  0x00007ffff699007b in Gui::TaskView::TaskDialogPython::getStandardButtons (this=0x55555a83aff0) at ../../src/Gui/TaskView/TaskDialogPython.cpp:739
#9  0x00007ffff6982922 in Gui::TaskView::TaskView::showDialog (this=0x555555f1db70, dlg=0x55555a83aff0) at ../../src/Gui/TaskView/TaskView.cpp:518
#10 0x00007ffff64aed84 in Gui::ControlSingleton::showDialog (this=0x555555dd6800, dlg=0x55555a83aff0) at ../../src/Gui/Control.cpp:170
#11 0x00007ffff6989aee in Gui::TaskView::ControlPy::showDialog (this=0x555555d3fd30, args=...) at ../../src/Gui/TaskView/TaskDialogPython.cpp:122
#12 0x00007ffff6992972 in Py::PythonExtension<Gui::TaskView::ControlPy>::method_varargs_call_handler (_self_and_name_tuple=0x7fff3c538200, _args=0x7fffeb6a5cf0) at ../../src/CXX/Python3/ExtensionOldType.hxx:309
#13 0x00007ffff30bd2ed in ??? () at /usr/lib/libpython3.12.so.1.0
#14 0x00007ffff309d50b in _PyObject_MakeTpCall () at /usr/lib/libpython3.12.so.1.0
#15 0x00007ffff2fa3dfa in ??? () at /usr/lib/libpython3.12.so.1.0
#16 0x00007ffff30ece06 in ??? () at /usr/lib/libpython3.12.so.1.0
#17 0x00007ffff6463d5b in Base::pyCall (callable=0x7fff3c5327c0, args=0x7fff3c531a00) at ../../src/Base/Interpreter.h:137
#18 0x00007ffff6ac65f4 in Gui::ViewProviderPythonFeatureImp::setEdit (this=0x55555a701100, ModNum=0) at ../../src/Gui/ViewProviderPythonFeature.cpp:371
#19 0x00007fff38fb5e9f in Gui::ViewProviderPythonFeatureT<PartGui::ViewProvider2DObject>::setEdit (this=0x55555a731d00, ModNum=0) at ../../src/Gui/ViewProviderPythonFeature.h:524
#20 0x00007ffff6a72951 in Gui::ViewProvider::startEditing (this=0x55555a731d00, ModNum=0) at ../../src/Gui/ViewProvider.cpp:134
#21 0x00007ffff6aa26e3 in Gui::ViewProviderDragger::startEditing (this=0x55555a731d00, mode=0) at ../../src/Gui/ViewProviderDragger.cpp:92
#22 0x00007ffff63d5c9e in Gui::Document::setEdit (this=0x5555584b1630, p=0x55555a731d00, ModNum=0, subname=0x0) at ../../src/Gui/Document.cpp:429
#23 0x00007ffff6455c1a in Gui::DocumentPy::setEdit (this=0x5555585b7820, args=0x7fff3c5318c0) at ../../src/Gui/DocumentPyImp.cpp:147
#24 0x00007ffff6451135 in Gui::DocumentPy::staticCallback_setEdit (self=0x5555585b7828, args=0x7fff3c5318c0) at src/Gui/DocumentPy.cpp:525
...

Full version info

OS: Arch Linux (XFCE/xfce)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.37330 +20 (Git)
Build type: Debug
Branch: integration
Hash: 6af3ce7cabae0c114039c94219259017db042d04
Python 3.12.3, Qt 5.15.13, Coin 4.0.2, Vtk 9.3.0, OCC 7.3.0
Locale: German/Germany (de_DE)
Installed mods: 
  * Manipulator 1.5.7
  * ThreadProfile 1.84.0
  * DynamicData 2.62.0
  * Assembly3.backup1715897154.0970206 0.11.3 (Disabled)
  * lattice2 1.0.0
  * 3D_Printing_Tools
  * freecad.gears 1.2.0
  * fasteners 0.4.21

Subproject(s) affected?

PartDesign

Anything else?

The last Arch Linux updates to python 3.12 caused some problems with pyside2 because it's not officially supported. With some patches it works, but something has changed with the enum handling.

This could be a regression from #13611. The getStandardButtons now returns a QFlags<QMessageBox::StandardButton> aka QMessageBox::StandardButtons, but the Shiboken::Enum::check() function returns false, triggering an assert in Shiboken::Enum::getValue().

If I compile against Qt6 with pyside6, the assert is not triggered.

I haven't figured out who really caused this. It could be libshiboken, python 3.12, #13611 or a combination of those.

If I patch the PythonWrapper::toEnum() function to

qsizetype PythonWrapper::toEnum(PyObject* pyPtr)
{
#if defined (HAVE_SHIBOKEN) && defined(HAVE_PYSIDE)
    auto longObj = PyNumber_Long(pyPtr);
    if (longObj) {
        long result = PyLong_AsLong(longObj);
        Py_DECREF(longObj);
        return result;
    }
    return Shiboken::Enum::getValue(pyPtr);
#else
    return toEnum(Py::Object(pyPtr));
#endif
}

it will work because flags are convertable to a number by PyNumber_Long().

Code of Conduct

  • I agree to follow this project's Code of Conduct
@maxwxyz maxwxyz added Bug This issue or PR is related to a bug WB Part Design Related to the Part Design Workbench OS: Linux labels May 20, 2024
@wwmayer
Copy link
Contributor

wwmayer commented May 22, 2024

when calling the "Involute Gear" command from PartDesign

Does this only happen with this command or with others too that load a task dialog (e.g. Pad, Pocket, ...)?

@wwmayer
Copy link
Contributor

wwmayer commented May 22, 2024

I wonder whether the two toEnum() functions really have to be that complicated and that we can't simply write:

qsizetype PythonWrapper::toEnum(PyObject* pyPtr)
{
    try {
        Py::Long longObj(PyNumber_Long(pyPtr), true);
        return longObj.as_long();
    }
    catch (Py::Exception&) {
        Base::PyException e;
        e.ReportException();
        return 0;
    }
}

qsizetype PythonWrapper::toEnum(const Py::Object& pyobject)
{
    return toEnum(pyobject.ptr());
}

This will have the same effect like the casts to an int on Python side:
return int(QtGui.QDialogButtonBox.Ok | QtGui.QDialogButtonBox.Cancel)

In #13611 it was claimed that this cast caused various problems but when looking at the mentioned issues all of them had other reasons and thus I doubt that Shiboken::Enum::getValue is needed at all.

@wwmayer
Copy link
Contributor

wwmayer commented May 22, 2024

I haven't figured out who really caused this. It could be libshiboken

For some reason the passed value to Shiboken::Enum::getValue is not of the expected type in your case and that's why check() causes the assert to fail:

long Shiboken::Enum::getValue(PyObject *enumItem)
{
    assert(Shiboken::Enum::check(enumItem));
...
}

@NomAnor
Copy link
Contributor Author

NomAnor commented May 22, 2024

when calling the "Involute Gear" command from PartDesign

Does this only happen with this command or with others too that load a task dialog (e.g. Pad, Pocket, ...)?

From the code I think this will happen with any dialog that is created in python, but I haven't checked.

Using PyNumber_Long will fixed it. There are multiple helper functions to convert from python to c++. I a sense, using toEnum in getStandardButtons is just wrong because it returns a QFlags not an enum so it should call a toFlags function. On the other hand toEnum returns an int not an enum which should probably a template function with a static_cast<T>() in it.

I haven't figured out who really caused this. It could be libshiboken

For some reason the passed value to Shiboken::Enum::getValue is not of the expected type in your case and that's why check() causes the assert to fail:

That was clear, but it worked before and I supect some weired interactions with package updates. But It could also be just the PR that changed the int casts.

I can test with your simplified function, I'm sure that will work.

@NomAnor
Copy link
Contributor Author

NomAnor commented May 23, 2024

@wwmayer I tested the simplified functions you posted and the crash is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This issue or PR is related to a bug OS: Linux WB Part Design Related to the Part Design Workbench
Projects
None yet
Development

No branches or pull requests

3 participants