[BUG] Complex function argument lists causing MSVC to fail

[jonathanhogg]

Describe the bug

I suspect this is a variant of the bug described in #5290 as I am seeing the same compiler error:

      src/flitter/render/physics.c(22571): error C2039: '__pyx_mstate_global': is not a member of '__pyx_mstate'
      src/flitter/render/physics.c(3910): note: see declaration of '__pyx_mstate'

The line that it is exploding on is

  if ((CYTHON_USE_TYPE_SLOTS && CYTHON_USE_PYTYPE_LOOKUP) && likely(!__pyx_ptype___pyx_scope_struct____Pyx_CFunc_3ffa58__double__lParenPhysicsSystem__comma_list__comma_list__comma_list__comma_list__comma_list__comma_list__comma_int__comma_bint__comma_bint__comma_double__comma_double__comma_double__comma_double__comma_double__rParen__etc_to_py_4self_9particles_11non_anchors_15particle_forces_13matrix_forces_15specific_forces_8barriers_10dimensions_8realtime_11extra_frame_14speed_of_light_4time_10start_time_10resolution_5clock->tp_dictoffset && __pyx_ptype___pyx_scope_struct____Pyx_CFunc_3ffa58__double__lParenPhysicsSystem__comma_list__comma_list__comma_list__comma_list__comma_list__comma_list__comma_int__comma_bint__comma_bint__comma_double__comma_double__comma_double__comma_double__comma_double__rParen__etc_to_py_4self_9particles_11non_anchors_15particle_forces_13matrix_forces_15specific_forces_8barriers_10dimensions_8realtime_11extra_frame_14speed_of_light_4time_10start_time_10resolution_5clock->tp_getattro == PyObject_GenericGetAttr)) {

and this looks to be generated indirectly from this function signature:

cdef class PhysicsSystem:

    ...

    cdef double calculate(self, list particles, list non_anchors, list particle_forces, list matrix_forces, list specific_forces, list barriers,
                          int dimensions, bint realtime, bint extra_frame, double speed_of_light,
                          double time, double start_time, double resolution, double clock):
        ...

Now I recognise that this is a lot of parameters, but it doesn't seem excessive.

Code to reproduce the behaviour:

No response

Expected behaviour

No response

OS

Windows 11 64-bit

Python version

3.12.2

Cython version

3.0.8

Additional context

I will attempt to reduce this to a smaller, full, failing example, but I'm a bit pressed for time this morning.

[jonathanhogg]

OK, tried a test of combining all of the arguments together into a single args tuple a la:

    cdef double calculate(self, tuple args):
        cdef list particles, non_anchors, particle_forces, matrix_forces, specific_forces, barriers
        cdef int dimensions
        cdef bint realtime, extra_frame
        cdef double speed_of_light, time, start_time, resolution, clock
        particles, non_anchors, particle_forces, matrix_forces, specific_forces, barriers, dimensions, realtime, extra_frame, speed_of_light, time, start_time, resolution, clock = args
        ...

(and similarly combining at the call site) and this compiles and seems to be OK.

[jonathanhogg]

Oh! The failing MSVC is:

error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.38.33130\\bin\\HostX86\\x64\\cl.exe' failed with exit code 2

In case the version number is important.

[jonathanhogg]

Ah. The name mangling in 3.0.9 has become even more verbose it seems. I had removed a bunch of parameters in order to get this to compile and now it's failing again…

I was down to just:

cdef class PhysicsSystem:
    ...
    cdef double calculate(self, tuple objects, bint realtime, bint extra_frame, double speed_of_light, double time, double clock):
        ...

which doesn't feel like an excessive method signature, but is uncompilable on Windows.

Would it make sense for the name mangler to truncate long names and add a hash of the full string on the end instead?

[jonathanhogg]

I see that there already is a cap_length() function in PyrexTypes.py, but I assume that is not being used in this particular instance.

I'd like to dig further, but I'm not sure I can take on learning another codebase on top of everything I'm already doing at the moment…

[lorentzenchr]

@da-woods If you could have a look that would be great. I hit the same error message in scikit-learn/scikit-learn#28638.

[da-woods]

This is probably fixed by #6104 but I'm struggling to reproduce the issue from the information here

[jonathanhogg]

OK, I've managed to boil my code down into a simple failing example:

src.zip

Error on Windows 11:

PS Microsoft.PowerShell.Core\FileSystem::\\...> cythonize.exe --inplace .\src\flitter\render\physics.pyx
Compiling \\...\src\flitter\render\physics.pyx because it changed.
[1/1] Cythonizing \\...\src\flitter\render\physics.pyx
physics.c
\\...\src\flitter\render\physics.c(5332): error C2039: '__pyx_mstate_global': is not a member of '__pyx_mstate'
\\...\src\flitter\render\physics.c(2453): note: see declaration of '__pyx_mstate'
error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.38.33130\\bin\\HostX86\\x64\\cl.exe' failed with exit code 2```

I should have done this ages ago, sorry, but Other Stuff got in the way!

The specific missing piece of information (which I only discovered on trying to repro it) is that the problem is occurring because I am passing a pointer to the function to another function.

Edit: In fact, now that I dig into this, if I redefine the wrapped function as cpdef, the problem goes away. So it is specifically passing a pointer to a cdef function to another.

Which, of course, gives me an easy workaround for it…

Edit 2: Should this have ever worked anyway? Now that I think about it for more than 1 minute, this feels like obtuse behaviour on my part. Declaring it as cdef surely means it isn't meant to be callable from Python? So passing it as an argument to a Python function and forcing a wrapper to be constructed on the fly sounds like something that I should have been discouraged from doing.

[da-woods]

Thanks for the reproducer. My initial attempt improved things but wasn't sufficient alone; the current version should fix it I think (I haven't tested on MSVC but I've looked at the source and it's a lot better).

Edit 2: Should this have ever worked anyway? Now that I think about it for more than 1 minute, this feels like obtuse behaviour on my part. Declaring it as cdef surely means it isn't meant to be callable from Python? So passing it as an argument to a Python function and forcing a wrapper to be constructed on the fly sounds like something that I should have been discouraged from doing.

Yeah I kind of agree with this. Generally I think implicit type conversions are a mistake (for anything non-trivial), and we probably should have kept cpdef as the one way to do it. In this case the issue is that you think you've avoided the overhead of a def function, but it's been implicitly added back.

However, since the conversion is there it has to work reliably.

[jonathanhogg]

I honestly feel bad about raising this as a bug now, because this was dumb code on my part. I'm going with a cpdef (or even a def) on this now.

Still, as you say, if it's to be allowed I guess it should at least work…

[lorentzenchr]

I honestly feel bad about raising this as a bug now, because this was dumb code on my part.

I‘m very glad you raised it as I hit the same error without this cdef/ cpdef function pointer.