gh-111178: fix UBSan failures in Objects/exceptions.c
#128154
Conversation
picnixz
commented
Dec 21, 2024
•
- Issue: UBSan: Calling a function through pointer to incorrect function type is undefined behavior #111178
This also align the naming convention `_Py[...]_CAST(op)` where only an assert-only type check is performed.
# Conflicts: # Objects/exceptions.c
Modules/exceptions.c
Modules/exceptions.cObjects/exceptions.c
|
@encukou I've used static inline functions here (because having the |
| 0, BaseException_dealloc, 0, 0, 0, 0, 0, 0, 0, \ | ||
| 0, 0, 0, 0, 0, 0, 0, \ | ||
| Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, \ | ||
| PyDoc_STR(EXCDOC), (traverseproc)BaseException_traverse, \ | ||
| (inquiry)BaseException_clear, 0, 0, 0, 0, 0, 0, 0, &_ ## EXCBASE, \ | ||
| PyDoc_STR(EXCDOC), BaseException_traverse, \ | ||
| BaseException_clear, 0, 0, 0, 0, 0, 0, 0, &_ ## EXCBASE, \ | ||
| 0, 0, 0, offsetof(PyBaseExceptionObject, dict), \ | ||
| (initproc)BaseException_init, 0, BaseException_new,\ | ||
| BaseException_init, 0, BaseException_new,\ |
There was a problem hiding this comment.
While you're touching these, (introducing conflicts with any in-flight PR, and adding yourself to git blame), could you switch to named initializers?
Feel free to do it in a follow-up PR if you want to merge this one soon.
There was a problem hiding this comment.
I think I've said it in another PR, but Serhiy asked for a discussion on DPO (which never happened I think) in #127679 and you asked for it to be included in PEP-7 first so I left those out (especially for static types like these; I think I touched some heap types and internal ones never exposed, but I don't think I touched something that was publicly exposed).
There was a problem hiding this comment.
Feel free to do it in a follow-up PR if you want to merge this one soon.
For UBSan failures, I expect them to be merged whenever it's fine to merge them. I just have a bunch of branches that I ignore when I do git branch (otherwise I just have too many of them locally). And I don't mind solving conflicts
There was a problem hiding this comment.
For changing them all at once, it IMO should definitely go in PEP-7. In the usual case there's nothing wrong with the "old" syntax, and we generally avoid style-only changes.
In this case though, the style is a lot less readable than usual -- it's missing the comment column, and it has more items per line. Since you are touching this anyway (so backports will conflict, etc.), this is the time to clean it up.
I'll merge this PR; feel free to follow up.