There was a test failure under PyPy:

[oscarbenjamin]

There was a test failure under PyPy in gh-323:

File "/Users/runner/hostedtoolcache/PyPy/3.11.13/x64/lib/pypy3.11/site-packages/flint/types/fq_default.pypy311-pp73-darwin.so", line 270, in flint.types.fq_default.fq_default_ctx.gen
Failed example:
    gf.gen()
Expected:
    w
Got:
    <BLANKLINE>

The change here does not look like it could cause that so maybe there is something undefined here.

Originally posted by @oscarbenjamin in #323 (comment)

[oscarbenjamin]

There was a test failure under PyPy:

I can't reproduce it. I've tested under pypy and all tests pass.

The failed doctest is just something like:

from flint import fq_default_ctx

gf = fq_default_ctx(5, 2, var="w")
g = gf.gen()
assert str(g) == 'w'
assert repr(g) == 'w'

I've run that under PyPy 1000 times though and don't see the failure.

The implementation here creates a polynomial and tries to print that:

python-flint/src/flint/types/fq_default.pyx

Lines 505 to 506 in 0a59f53

	def str(self):
	return self.polynomial().str(var=self.ctx.var.decode())

The polynomial printing code does not check if var is a non-empty or valid string:

python-flint/src/flint/flint_base/flint_base.pyx

Lines 191 to 224 in 0a59f53

	def str(self, bint ascending=False, var="x", *args, **kwargs):
	"""
	Convert to a human-readable string (generic implementation for
	all polynomial types).
	If *ascending* is *True*, the monomials are output from low degree to
	high, otherwise from high to low.
	"""
	coeffs = [c.str(*args, **kwargs) for c in self]
	if not coeffs:
	return "0"
	s = []
	coeffs = enumerate(coeffs)
	if not ascending:
	coeffs = reversed(list(coeffs))
	for i, c in coeffs:
	if c == "0":
	continue
	else:
	if c.startswith("-") or (" " in c):
	c = "(" + c + ")"
	if i == 0:
	s.append("%s" % c)
	elif i == 1:
	if c == "1":
	s.append(var)
	else:
	s.append(f"{c}*{var}")
	else:
	if c == "1":
	s.append(f"{var}^{i}")
	else:
	s.append(f"{c}*{var}^{i}")
	return " + ".join(s)

It is maybe worth adding a check there so that we could see if that is what failed if the test ever fails again in future.

The var is coming from self.ctx.var.decode() which is:

python-flint/src/flint/types/fq_default.pxd

Lines 25 to 28 in 0a59f53

	cdef class fq_default_ctx:
	cdef fq_default_ctx_t val
	cdef readonly char *var
	cdef bint _initialized

and is initialised here:

python-flint/src/flint/types/fq_default.pyx

Lines 65 to 85 in 0a59f53

	@staticmethod
	def _parse_input_var(var):
	# If no variable is given, use x
	if var is None:
	var = b"z"
	# Encode to bytes for cython to parse
	if isinstance(var, str):
	var = var.encode()
	# TODO: Flint only wants one-character inputs
	if len(var) > 1:
	raise ValueError("variable for GF(p^k) generator can only be one character")
	return var
	def __init__(self, p=None, degree=None, var=None, modulus=None, fq_type=fq_default_type.DEFAULT,
	check_prime=True, check_modulus=True):
	# Ensure the var used for the generator of GF(p^d) is a single byte
	var = self._parse_input_var(var)
	self.var = var

I'm a bit suspicious about just assigning a Python bytes object to a char*.

Is that making a copy or possibly just storing a pointer?

The test failure is under PyPy so it is possible that this takes a different codepath in the Cython-generated C code.

Looking in the generated C code in build/src/flint/types/fq_default.pypy311-pp73-x86_64-linux-gnu.so.p/src/flint/types/fq_default.pyx.c there is:

  /* "flint/types/fq_default.pyx":85
 *         # Ensure the var used for the generator of GF(p^d) is a single byte
 *         var = self._parse_input_var(var)
 *         self.var = var             # <<<<<<<<<<<<<<
 * 
 *         # Ensure the fq_type is an integer between 0, 5 -- we allow users to
*/
  __pyx_t_4 = __Pyx_PyObject_AsWritableString(__pyx_v_var); if (unlikely((!__pyx_t_4) && PyErr_Occurred())) __PYX_ERR(0, 85, __pyx_L1_error)
  __pyx_v_self->var = __pyx_t_4;

Chasing the __Pyx_PyObject_AsWritableString macro leads to:

static CYTHON_INLINE const char* __Pyx_PyObject_AsStringAndSize(PyObject* o, Py_ssize_t *length) {
#if __PYX_DEFAULT_STRING_ENCODING_IS_ASCII || __PYX_DEFAULT_STRING_ENCODING_IS_UTF8
    if (PyUnicode_Check(o)) {
        return __Pyx_PyUnicode_AsStringAndSize(o, length);
    } else
#endif
    if (PyByteArray_Check(o)) {
#if (CYTHON_ASSUME_SAFE_SIZE && CYTHON_ASSUME_SAFE_MACROS) || (CYTHON_COMPILING_IN_PYPY && (defined(PyByteArray_AS_STRING) && defined(PyByteArray_GET_SIZE)))
        *length = PyByteArray_GET_SIZE(o);
        return PyByteArray_AS_STRING(o);
#else
        *length = PyByteArray_Size(o);
        if (*length == -1) return NULL;
        return PyByteArray_AsString(o);
#endif
    } else
    {
        char* result;
        int r = PyBytes_AsStringAndSize(o, &result, length);
        if (unlikely(r < 0)) {
            return NULL;
        } else {
            return result;
        }
    }
}

The CYTHON_COMPILING_IN_PYPY define looks potentially relevant.

The macro PyByteArray_AS_STRING is defined here:
https://github.com/python/cpython/blob/d7b9ea5cab984497526fcc0b988e4eb5988c1e88/Include/cpython/bytearrayobject.h#L5-L28
It looks like this just returns the raw pointer i.e. it does not allocate a new string.

My guess then is that the fq_default_ctx ends up holding a pointer to the buffer for a bytes object that was deallocated. The memory was overwritten with something and then self.ctx.var.decode() ends up having a pointer to a null byte "'\0'" which decode turns into an empty string.

I guess we need a malloc/copy here or on the other hand if as noted in the comments FLINT only allows a single character string we could change it so that var is char[2] rather than a char* and just copy the first byte in.

Also _parse_input_var should check that the string is not of length 0.

[da-woods]

I'd be very suspicious of that assignment to char*. The Python string ultimately owns the memory so you need to ensure that it outlives that char*. That's often fine for char* function locals but not so fine for class attributes.

There's probably some cases where it'll work (the val is None case will end up pointing to a cached string for example) but a lot of others where it won't.

So I don't think this is specifically a PyPy issue here

(I suspect a short string like w might end up interned by the Python interpreter so you get away with it. But that's not something to rely on)

[oscarbenjamin]

Thanks, I forgot about this issue. Yes the assignment to char* I think is unsafe. My notes above were just me trying to figure out what it does which I think is basically just

cdef bytes bytestring
cdef char *var
var = bytestring->ob_start

It isn't safe to store that pointer if we don't also hold a reference to the bytestring.

Looking at it I think that var should just be a Python string object rather than char * though. There are many places doing self.var.decode() and char* is only needed at context initialisation time for functions like fq_default_ctx_init_type.

Assuming it works as shown above I think that while we hold a reference to the bytes object it should be safe to pass a bytes from Cython into a C function that expects const char* so that FLINT can malloc/copy and attach it to the fq_default_ctx struct.

[da-woods]

Assuming it works as shown above I think that while we hold a reference to the bytes object it should be safe to pass a bytes from Cython into a C function that expects const char* so that FLINT can malloc/copy and attach it to the fq_default_ctx struct.

Yes that should be safe (and is the intention of the bytes to const char* conversion)

[oscarbenjamin]

Thanks. I've fixed this by going for the safer option of just storing a normal Python string that can be reference counted.

It isn't clearly documented in FLINT but the char* that gets passed in to fq_default_ctx_init_* functions is copied into a newly malloc'd buffer which is then attached to the struct and gets cleared when the struct is freed:

python-flint/src/flint/types/fq_default.pyx

Lines 45 to 48 in 80f941f

	def __dealloc__(self):
	if self._initialized:
	fq_default_ctx_clear(self.val)
	self._initialized = False

By the way @da-woods since you are here do you know if there are any guarantees about how many times __dealloc__ is called or whether it could be called from two separate threads (in free-threading)?

[da-woods]

The user code in __dealloc__ is called exactly once.

I don't think you can reason too much in advance about which thread might run it but it should only be one thread.

[oscarbenjamin]

Thanks, that is reassuring. In our case here it doesn't matter which thread but we want to call that *_clear function exactly once for each object so that is the precise guarantee that is needed.