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

[BUG] from libcpp.vector import * #5562

Closed
imphil opened this issue Jul 25, 2023 · 4 comments · Fixed by #5561
Closed

[BUG] from libcpp.vector import * #5562

imphil opened this issue Jul 25, 2023 · 4 comments · Fixed by #5561

Comments

@imphil
Copy link
Contributor

imphil commented Jul 25, 2023

Describe the bug

The line from libcpp.vector import * in the testcase below leads to GCC failing to compile the test with the following error:

Compiler output for module vector_include:
vector_include.cpp: In function ‘int __pyx_import_star_set(PyObject*, PyObject*, char*)’:
vector_include.cpp:2013:47: error: lvalue required as left operand of assignment
     PY_SSIZE_T_MAX = __Pyx_PyIndex_AsSsize_t(o); if (unlikely((PY_SSIZE_T_MAX == (Py_ssize_t)-1) && PyErr_Occurred())) __PYX_ERR(0, 63, __pyx_L2_error)
                                               ^

The compile error makes sense, as Cython 3.0.0 generates the following code:

static int __pyx_import_star_set(PyObject *o, PyObject* py_name, char *name) {
  static const char* internal_type_names[] = {
    "X",
    "__pyx_ctuple_Py_ssize_t",
    "__pyx_ctuple_Py_ssize_t_struct",
    "vector",
    0
  };
  const char** type_name = internal_type_names;
  while (*type_name) {
    if (__Pyx_StrEq(name, *type_name)) {
      PyErr_Format(PyExc_TypeError, "Cannot overwrite C type %s", name);
      goto bad;
    }
    type_name++;
  }
  if (0);
  else if (__Pyx_StrEq(name, "PY_SSIZE_T_MAX")) {
    PY_SSIZE_T_MAX = __Pyx_PyIndex_AsSsize_t(o); if (unlikely((PY_SSIZE_T_MAX == (Py_ssize_t)-1) && PyErr_Occurred())) __PYX_ERR(0, 63, __pyx_L2_error)
  }
  else {
    if (PyObject_SetAttr(__pyx_m, py_name, o) < 0) goto bad;
  }
  return 0;
  __pyx_L2_error:;
  __Pyx_AddTraceback("test_return_vector", __pyx_clineno, __pyx_lineno, __pyx_filename);
  bad:
  return -1;
}

Code to reproduce the behaviour:

# vector_include.pyx
from libcpp.vector cimport *
# The error goes away if I comment out the following line:
from libcpp.vector import *

cdef extern from "vector_include.h":
    vector[int] get_vector()

my_vector = get_vector()
// vector_include.h
#pragma once

#include <vector>

std::vector<int> get_vector()
{
  return std::vector<int>(17);
}

A runnable test showing this behavior is available in #5561 for easier reproduction.

Expected behaviour

No response

OS

Linux

Python version

3.9.16

Cython version

3.0.0

Additional context

This code (well, the code this was reduced from) worked fine in Cython 3.0a1 (way.... back then, I know). If necessary I can try to bisect this further.

@da-woods
Copy link
Contributor

Duplicate of #4931 (but thanks for the report!)

This isn't to do with the from libcpp.vector import * (which I'm pretty sure won't work since libcpp.vector isn't a Python module)

I think it's really from anything import *.

The trouble is Cython declares an extern int PY_SSIZE_T_MAX somewhere in internal utility code used to generate conversion of vectors to/from Python types. Generally import * tries to overwrite C variables (on the basis that if the module defined Py_SSIZE_T_MAX then you'd expect it to be updated). This generates invalid code since Py_SSIZE_T_MAX isn't actually writeable.

This really shouldn't happen for things defined in utility code and maybe should be more limited generally. For example maybe the extern variable should be declared const and thus not be replaceable..

imphil added a commit to imphil/cython that referenced this issue Jul 26, 2023
Follow the lead of `Cython/Includes/cpython/pyport.pxd` to declare
`PY_SSIZE_T_MIN` and `PY_SSIZE_T_MAX` as `const`. This prevents Cython
from trying to override or assign to this "variable" when doing wildcard
imports.

Add a test that shows this behavior in one example.

Fixes cython#5562
imphil added a commit to imphil/cython that referenced this issue Jul 26, 2023
Follow the lead of `Cython/Includes/cpython/pyport.pxd` to declare
`PY_SSIZE_T_MIN` and `PY_SSIZE_T_MAX` as `const`. This prevents Cython
from trying to override or assign to this "variable" when doing wildcard
imports.

Add a test that shows this behavior in one example.

Fixes cython#5562
@imphil
Copy link
Contributor Author

imphil commented Jul 26, 2023

@da-woods Thanks for your input, and thanks for the pointer to the underlying general issue.

For this particular issue I found that (thanks to your input!) a more fine-grained solution does the trick: make PY_SSIZE_T_MAX const. I pushed the code change to #5561.

We have an existing code base that uses libcpp.vector and wildcard imports of other modules heavily and this issue prevents us updating to Cython 3, so I'd appreciate it if we can get the more focused fix in without blocking on the more general solution (which I'm happy to contribute to, if you give me a pointer where to start!).

@da-woods da-woods reopened this Jul 26, 2023
@da-woods
Copy link
Contributor

da-woods commented Jul 26, 2023

Will reopen so we have something to link your specific fix. I think it's an improvement anyway so happy to merge it ahead of a more general fix.

@da-woods
Copy link
Contributor

In terms of the more general fix: I think the relevant function is ModuleNode.generate_import_star (in ModuleNode.py). It iterates over a list of entries and assigns to them. I don't know off the top of my head how you'd tell that an entry comes from utility code. It might be possible just to look up entry.scope.name and tell there. Or it might be something where you'd have to set a new attribute during the ModuleNode.merge_in process.

imphil added a commit to imphil/cython that referenced this issue Jul 26, 2023
Follow the lead of `Cython/Includes/cpython/pyport.pxd` to declare
`PY_SSIZE_T_MIN` and `PY_SSIZE_T_MAX` as `const`. This prevents Cython
from trying to override or assign to this "variable" when doing wildcard
imports.

Add a test that shows this behavior in one example.

Fixes cython#5562
da-woods pushed a commit that referenced this issue Jul 26, 2023
Follow the lead of `Cython/Includes/cpython/pyport.pxd` to declare
`PY_SSIZE_T_MIN` and `PY_SSIZE_T_MAX` as `const`. This prevents Cython
from trying to override or assign to this "variable" when doing wildcard
imports.

Add a test that shows this behavior in one example.

Fixes #5562
@da-woods da-woods added this to the 3.0.1 milestone Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants