[libclang/python] Fix visitor callback return data type - #221024

@llvm/pr-subscribers-clang

Author: Milad Fa (miladfarca)

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index bc00dd770ce3b..4423096085535 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -71,6 +71,7 @@
     byref,
     c_char_p,
     c_int,
+    c_long,
     c_longlong,
     c_uint,
     c_ulong,
@@ -4117,8 +4118,8 @@ def set_property(self, property, value):
 translation_unit_includes_callback = CFUNCTYPE(
     None, c_object_p, POINTER(SourceLocation), c_uint, py_object
 )
-cursor_visit_callback = CFUNCTYPE(c_int, Cursor, Cursor, py_object)
-fields_visit_callback = CFUNCTYPE(c_int, Cursor, py_object)
+cursor_visit_callback = CFUNCTYPE(c_long, Cursor, Cursor, py_object)
+fields_visit_callback = CFUNCTYPE(c_long, Cursor, py_object)
 
 # Functions strictly alphabetical order.
 FUNCTION_LIST: list[LibFunc] = [

Perhaps I'm misunderstanding something, but the visitor should return instances of CXChildVisitResult, which is a bare enum with no explicit type and only 3 variants, so a regular int should be enough. Why would this need a long?

https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#7

For an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration. If no integral type can represent all the enumerator values, the enumeration is ill-formed.
It is implementation-defined which integral type is used as the underlying type except that the underlying type shall not be larger than int unless the value of an enumerator cannot fit in an int or unsigned int.
If the [enumerator-list](https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#nt:enumerator-list) is empty, the underlying type is as if the enumeration had a single enumerator with value 0.

Also adding @Endilll because C++ isn't my strong suite

Perhaps I'm misunderstanding something, but the visitor should return instances of CXChildVisitResult, which is a bare enum with no explicit type and only 3 variants, so a regular int should be enough. Why would this need a long?

The issue is that when this enum is put into a register that serves as the return slot, c_int preserves the upper four bytes of that register, which causes them to be garbage alongside the actual value in the lower 4 bytes. So the fix definitely makes sense on some level, but I wonder why ctypes does not sign-extend c_int on s390x to satisfy their psABI.

@miladfarca can you raise or find an existing issue about this on ctypes side, and put it as a comment in the code? My hope is that this would be a somewhat temporary fix, until Python releases with this bug reach EOL. Also, can we limit this workaround to s390x? I guess ctypes has less bugs on mainstream platforms, because I haven't seen other platform complaining about this.

For the reference, I dug up the following in s390x psABI version 1.7 (1.2.5 "Return Values"):

Any integer type with 64 or fewer bits, including _BitInt(𝑁) with 𝑁 ≤ 64, _Bool,
as well as any enum type, is returned in r2. The return value is zero- or sign-extended
to 64 bits, as appropriate.

@DeinAlptraum It's correct that the type is an enum however similar to @Endilll's explanation, ctypes has no concept of an enum. They get passed a data size (8/4/2 etc.) and in this case all they see is a 4 byte data type which per s390x ABI (correctly pasted above) and libffi contract, needs to be sign or zero extended to a full register size.

I have also created an issue on cpython upstream and have made this patch s390x specific:
python/cpython#156933

I have added a release note as well as a test, the crash itself came from V8's metagen on s390x.

LGMT, but could you confirm again this is what you imaged @AaronBallman ?

@AaronBallman not a problem, I've made it public.

I've merged a fix for this upstream, which will be available in Python 3.13.16 and 3.14.8 sometime in the next week or so (see PEPs 719 and 745 for the release schedule).