◐ Shell
reader mode source ↗
Skip to content

gh-111178: Generate correct signature for most self converters#128447

Merged
erlend-aasland merged 6 commits into
python:mainfrom
erlend-aasland:ubsan/clinic-self-converters
Jan 20, 2025
Merged

gh-111178: Generate correct signature for most self converters#128447
erlend-aasland merged 6 commits into
python:mainfrom
erlend-aasland:ubsan/clinic-self-converters

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jan 3, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

@erlend-aasland erlend-aasland requested a review from picnixz January 3, 2025 13:29
@bedevere-app bedevere-app Bot mentioned this pull request Jan 3, 2025
Closed
@erlend-aasland

This comment was marked as resolved.

Sign in to view
@erlend-aasland

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
Contributor Author

WDYT, @picnixz?

@picnixz

picnixz commented Jan 3, 2025

Copy link
Copy Markdown
Member

I think this is what I had in mind. I'm no more on my dev session but I'll pick your branch, create a branch with all my known UBSan fixes branches and then I'll need to manually address the remaining ones.

AFAICT, no new _impl functions were added so performances shouldn't be affected I think? (it's only pointer casts for the compiler). I'll review more in details tomorrow but thank you for the work!

@erlend-aasland

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
Contributor Author

Yeah, this PR only makes sure we're using PyObject pointers in the PyMethodDef "entries". No new [clinic input] sections, no new _impl functions. Performance should not be affected.

@erlend-aasland

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
Contributor Author

I'll leave this in draft so we don't create a review rush.

@erlend-aasland

erlend-aasland commented Jan 19, 2025

Copy link
Copy Markdown
Contributor Author

Friendly ping, @picnixz!

@picnixz

picnixz commented Jan 20, 2025

Copy link
Copy Markdown
Member

Sorry for the late reply. I checked what I still need to do for other UBSan failures and there are quite a lot of places to fix. Here's what I can suggest: we first merge your PR and then we continue fixing the rest of UBSan failures. I wanted to delay this PR until the other failures are patched but there just too many errors in the clang output that it's hard to know where to start.

So, I think we can first fix those now and then continue the work normally. I'll also tag @vstinner, @encukou and @ZeroIntensity who worked with me on that matter to hear their thoughts.

encukou
encukou approved these changes Jan 20, 2025
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

+1, let's not delay this step.

@picnixz

picnixz commented Jan 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Not sure if this is meant to be covered by this PR but:

static PyObject *
list_append(PyListObject *self, PyObject *object)
{
    PyObject *return_value = NULL;

    Py_BEGIN_CRITICAL_SECTION(self);
    return_value = list_append_impl((PyListObject *)self, object);
    Py_END_CRITICAL_SECTION();

    return return_value;
}

has not been changed. list_append (in listobject.c.h) should accept a PyObject *. Same for list_remove and others.

@picnixz

picnixz commented Jan 20, 2025

Copy link
Copy Markdown
Member

Also, the subsequent casts:

#define PY_LIST_CLEAR_METHODDEF    \
    {"clear", (PyCFunction)py_list_clear, METH_NOARGS, py_list_clear__doc__},

for

static PyObject *
py_list_clear(PyObject *self, PyObject *Py_UNUSED(ignored))
{
    PyObject *return_value = NULL;

    Py_BEGIN_CRITICAL_SECTION(self);
    return_value = py_list_clear_impl((PyListObject *)self);
    Py_END_CRITICAL_SECTION();

    return return_value;
}

are not needed but I don't know if it's possible to only remove the cast for the "fixed" methods or not.

@erlend-aasland

erlend-aasland commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Correct, @picnixz, I don't think this is a complete solution. But these kinds of changes have a huge impact. It is pretty easy to validate this change, for example, but if we bake in another Argument Clinic change, it will be harder to validate the generated output. IMO, it is better to address this in a follow-up PR.

@erlend-aasland erlend-aasland marked this pull request as ready for review January 20, 2025 11:38
6 hidden items Load more…
@erlend-aasland erlend-aasland merged commit 537296c into python:main Jan 20, 2025
@erlend-aasland erlend-aasland deleted the ubsan/clinic-self-converters branch January 20, 2025 11:40
@erlend-aasland erlend-aasland changed the title gh-111178: Generate correct signature for self converters Jan 20, 2025
@picnixz

picnixz commented Jan 20, 2025

Copy link
Copy Markdown
Member

, it will be harder to validate the generated output. IMO, it is better to address this in a follow-up PR.

Sure! I just wanted to be sure that you knew about this. But yes, otherwise I validate the current change (sorry for not having said it explicitly).

@picnixz

picnixz commented Jan 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

@erlend-aasland We're having some CI failures now but I think it's because main wasn't merged recently and clinic not regen.

@erlend-aasland

erlend-aasland commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Hm, yeah I guess we should've synced it before merging.

@picnixz

picnixz commented Jan 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Do you want to make a patch? (I can also do it if you don't have the time now)

@erlend-aasland

erlend-aasland commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Yes, I'm creating an amendment now. Compiling ... 🤖 🤖 🤖

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jan 20, 2025
@erlend-aasland
pythongh-111178: Regen clinic and fix exceptions.c post pythongh-128447
0241133
@erlend-aasland

erlend-aasland commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Amendment up: #129060.

@erlend-aasland erlend-aasland mentioned this pull request Jan 20, 2025
Merged
@vstinner

vstinner commented Jan 20, 2025

Copy link
Copy Markdown
Member

Nice enhancement, thanks.

erlend-aasland added a commit that referenced this pull request Jan 20, 2025
@erlend-aasland
gh-111178: Regen clinic and fix exceptions.c post gh-128447 (#129060)
da0f47c
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@erlend-aasland @srinivasreddy
pythongh-111178: Generate correct signature for most self converters (p… …
36ff53f 
…ython#128447)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@erlend-aasland @srinivasreddy
pythongh-111178: Regen clinic and fix exceptions.c post pythongh-128447… …
3e76a17 
… (python#129060)
@vstinner vstinner mentioned this pull request Feb 28, 2025
Merged
@picnixz picnixz mentioned this pull request Mar 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou

@picnixz picnixz Awaiting requested review from picnixz picnixz is a code owner

@corona10 corona10 Awaiting requested review from corona10

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

@tiran tiran Awaiting requested review from tiran

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@markshannon markshannon Awaiting requested review from markshannon

@1st1 1st1 Awaiting requested review from 1st1

@asvetlov asvetlov Awaiting requested review from asvetlov

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303

@willingc willingc Awaiting requested review from willingc

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland @picnixz @vstinner @encukou