Uh oh!
There was an error while loading. Please reload this page.
gh-111178: fix UBSan failures in Modules/_datetimemodule.c - #129774
Conversation
Uh oh!
There was an error while loading. Please reload this page.
picnixz
commented
Feb 7, 2025
Ah I'm sorry I put this one as ready for review but I learned that |
chris-eibl
left a comment
There was a problem hiding this comment.
Just nitpicks most probably related to mechanical changes.
At some places, the casts would not be needed and the PR could be kept smaller.
Most probably you'd still want the renamings like
time_utcoffset(PyObject*op, PyObject*Py_UNUSED(dummy))for consistency with the rest of the PR, even though there and at some other places no change would be needed at all.
Tried to locate them, hopefully no false positives - need a break now :)
So just happily ignore where not appropriate.
| static PyObject * | ||
| time_utcoffset(PyObject *self, PyObject *unused) { | ||
| time_utcoffset(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Before the PR, time_utcoffset already got a PyObject as first parameter and GET_TIME_TZINFO happily accepts it.
Maybe omit the PyTime_CAST to keep the diff smaller?
| static PyObject * | ||
| time_dst(PyObject *self, PyObject *unused) { | ||
| time_dst(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Similar to above: maybe omit the PyTime_CAST to keep the diff smaller?
| static PyObject * | ||
| time_tzname(PyDateTime_Time *self, PyObject *unused) { | ||
| time_tzname(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Similar to above: maybe omit the PyTime_CAST to keep the diff smaller?
| datetime_hour(PyDateTime_DateTime *self, void *unused) | ||
| datetime_hour(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
| datetime_minute(PyDateTime_DateTime *self, void *unused) | ||
| datetime_minute(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
| datetime_tzname(PyObject *self, PyObject *unused) { | ||
| return call_tzname(GET_DT_TZINFO(self), self); | ||
| datetime_tzname(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
| datetime_microsecond(PyDateTime_DateTime *self, void *unused) | ||
| datetime_microsecond(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
| datetime_ctime(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| return format_ctime((PyDateTime_Date *)self, | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
| datetime_getdate(PyDateTime_DateTime *self, PyObject *Py_UNUSED(ignored)) | ||
| datetime_getdate(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
| datetime_gettimetz(PyDateTime_DateTime *self, PyObject *Py_UNUSED(ignored)) | ||
| datetime_gettimetz(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
The DATE_GET_* macros would not need the PyDateTime_CAST.
picnixz
commented
Feb 9, 2025
The reason why I made the changes here is because the macros may be extended to static inline functions with runtime checks in the future and we definitely don't want to fill out missing pieces. For now, they don't do anything but ideally, the |
picnixz
commented
Feb 9, 2025
Ah! I remember what I did. I wrote a note saying that a postmerge task should be done where I eliminate the unnecessary casts in the GET macros so that we can add a check in the macro instead of in the getters. |
This PR fixes the UBSan failures and addresses some minor cosmetic changes. PEP-7 changes were not applied since they could scramble the diff but other semantic changes affecting the signature of touched functions may have been done.