Skip to content

gh-137725: Convert faulthandler to Argument Clinic #137726

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 13, 2025

@ZeroIntensity ZeroIntensity self-requested a review August 13, 2025 18:42
@ZeroIntensity
Copy link
Member

I don't understand this test failure. EXCEPTION_ACCESS_VIOLATION should be defined as 0xC0000005 on Windows, which isn't negative.

@colesbury
Copy link
Contributor

EXCEPTION_ACCESS_VIOLATION is >2**31 so when it's cast to a long, which is signed and only 32-bits on Windows, it becomes negative:

cpython/Modules/faulthandler.c

Lines 1330 to 1333 in a10152f

if (PyModule_AddIntConstant(module, "_EXCEPTION_ACCESS_VIOLATION",
EXCEPTION_ACCESS_VIOLATION)) {
return -1;
}

PyAPI_FUNC(int) PyModule_AddIntConstant(PyObject *, const char *, long);

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Aug 13, 2025

Ah, and it seems that PyArg_ParseTuple allows negative values for whatever reason (but AC doesn't):

static PyObject *
foo(PyObject *self, PyObject *args)
{
    unsigned int a;
    if (!PyArg_ParseTuple(args, "I", &a)) {
        return NULL;
    }
    printf("a: %u\n", a);

    Py_RETURN_NONE;
}
>>> foo(-1)
a: 4294967295

Yuck.

@vstinner
Copy link
Member Author

I fixed the Windows EXCEPTION constants by using PyLong_FromUnsignedLong(). Constants and _raise_exception() are only defined for test_faulthandler, so it's ok to no longer accept negative values.

@vstinner
Copy link
Member Author

Ok, the CI does now pass. The PR is now ready for a review :-) Sorry, I should have marked it as a draft in the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants