gh-148510: restore func_version check in _LOAD_ATTR_PROPERTY_FRAME#148528
gh-148510: restore func_version check in _LOAD_ATTR_PROPERTY_FRAME#148528NekoAsakura wants to merge 1 commit intopython:mainfrom
func_version check in _LOAD_ATTR_PROPERTY_FRAME#148528Conversation
NekoAsakura
requested review from
Fidget-Spinner,
markshannon,
savannahostrowski and
tomasr8
as code owners
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Just one thing that should be addressed. Thanks!
| assert((oparg & 1) == 0); | ||
| assert(Py_IS_TYPE(fget, &PyFunction_Type)); | ||
| PyFunctionObject *f = (PyFunctionObject *)fget; | ||
| EXIT_IF(f->func_version != func_version); |
There was a problem hiding this comment.
Note: this does not need an FT_ATOMIC_ load (check the rest of bytecodes.c). On FT, I suspect it's because function verison resets are stop-the-world (see Objects/funcobject.c)
| PyFunctionObject *func = (PyFunctionObject *)fget; | ||
| if (func->func_version != func_version) { |
There was a problem hiding this comment.
This needs a stronger invariant. There might be a case where fget is deallocated or deleted, and we are thus accessing a NULL pointer!
The reason why this is safe currently in the bytecodes interpreter but not here is that we have _GUARD_TYPE_VERSION in the bytecodes interpreter that guarantees that our cached function is alive.
While here, the _GUARD_TYPE_VERSION only sets the type version, and doesn't stop optimization if we detect something wrong.
To make this safe, you need to get the probable type in _GUARD_TYPE_VERSION, and also set things to contradiction+done if they don't already match the type_version in there. Since we record LOAD_ATTR_PROPERTY's type, this will then be safe to access if that passes.
3 participants
Do not merge.
Prerequisite: #148524
Mean +- std: [main] 12.8 ms +- 0.2 ms -> [branch] 12.3 ms +- 0.1 ms