Skip to content

GH-99298: Don't perform jumps before error handling #99299

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 10, 2022
Merged

GH-99298: Don't perform jumps before error handling #99299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
336f3e4
Don't perform jumps before error handling
brandtbucher Nov 9, 2022
02085ee
blurb add
brandtbucher Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2022-11-09-12-07-24.gh-issue-99298.NeArAJ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix an issue that could potentially cause incorrect error handling for some
bytecode instructions.
34 changes: 19 additions & 15 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,8 @@ dummy_func(
PyObject *name = GETITEM(names, oparg);
next_instr--;
if (_Py_Specialize_StoreAttr(owner, next_instr, name)) {
// "undo" the rewind so end up in the correct handler:
next_instr++;
Copy link
Member

Choose a reason for hiding this comment

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

Adding any code to a conditional block with just a goto adds extra branches.
It's not so important for these slower instructions, but its best avoided in general.

What we generally do is if (cond) goto fixup_error; where fixup_error does the necessary fix before jumping to error:. The C compiler will probably do this for us, but I think it best to be explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, see my comment at the top of this PR. _Py_Specialize_StoreAttr and _Py_Specialize_LoadAttr don't really need error handling at all, so the next thing I'm going to do it make them return void like the others in 3.12. Then we don't need the branch at all. :)

I'm hesitant to add additional labels and gotos to the 3.11 backport, especially since this is pretty cold code. Let me know if you think I should, though.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, leave this for this PR.

Maybe make a PR for 3.12 that removes the branch entirely?

goto error;
}
DISPATCH_SAME_OPARG();
Expand Down Expand Up @@ -1735,6 +1737,8 @@ dummy_func(
PyObject *name = GETITEM(names, oparg>>1);
next_instr--;
if (_Py_Specialize_LoadAttr(owner, next_instr, name)) {
// "undo" the rewind so end up in the correct handler:
next_instr++;
goto error;
}
DISPATCH_SAME_OPARG();
Expand Down Expand Up @@ -3144,7 +3148,6 @@ dummy_func(
PyObject *callable = PEEK(2);
DEOPT_IF(callable != (PyObject *)&PyUnicode_Type, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyObject *arg = TOP();
PyObject *res = PyObject_Str(arg);
Py_DECREF(arg);
Expand All @@ -3154,6 +3157,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3165,7 +3169,6 @@ dummy_func(
PyObject *callable = PEEK(2);
DEOPT_IF(callable != (PyObject *)&PyTuple_Type, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyObject *arg = TOP();
PyObject *res = PySequence_Tuple(arg);
Py_DECREF(arg);
Expand All @@ -3175,6 +3178,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3188,7 +3192,6 @@ dummy_func(
PyTypeObject *tp = (PyTypeObject *)callable;
DEOPT_IF(tp->tp_vectorcall == NULL, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
STACK_SHRINK(total_args);
PyObject *res = tp->tp_vectorcall((PyObject *)tp, stack_pointer,
total_args-kwnames_len, call_shape.kwnames);
Expand All @@ -3203,6 +3206,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3218,7 +3222,6 @@ dummy_func(
DEOPT_IF(!PyCFunction_CheckExact(callable), CALL);
DEOPT_IF(PyCFunction_GET_FLAGS(callable) != METH_O, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyCFunction cfunc = PyCFunction_GET_FUNCTION(callable);
// This is slower but CPython promises to check all non-vectorcall
// function calls.
Expand All @@ -3237,6 +3240,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3252,7 +3256,6 @@ dummy_func(
DEOPT_IF(PyCFunction_GET_FLAGS(callable) != METH_FASTCALL,
CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyCFunction cfunc = PyCFunction_GET_FUNCTION(callable);
STACK_SHRINK(total_args);
/* res = func(self, args, nargs) */
Expand All @@ -3277,6 +3280,7 @@ dummy_func(
*/
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3291,7 +3295,6 @@ dummy_func(
DEOPT_IF(PyCFunction_GET_FLAGS(callable) !=
(METH_FASTCALL | METH_KEYWORDS), CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
STACK_SHRINK(total_args);
/* res = func(self, args, nargs, kwnames) */
_PyCFunctionFastWithKeywords cfunc =
Expand All @@ -3316,6 +3319,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3331,7 +3335,6 @@ dummy_func(
PyInterpreterState *interp = _PyInterpreterState_GET();
DEOPT_IF(callable != interp->callable_cache.len, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyObject *arg = TOP();
Py_ssize_t len_i = PyObject_Length(arg);
if (len_i < 0) {
Expand All @@ -3347,6 +3350,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
}

// stack effect: (__0, __array[oparg] -- )
Expand All @@ -3361,7 +3365,6 @@ dummy_func(
PyInterpreterState *interp = _PyInterpreterState_GET();
DEOPT_IF(callable != interp->callable_cache.isinstance, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyObject *cls = POP();
PyObject *inst = TOP();
int retval = PyObject_IsInstance(inst, cls);
Expand All @@ -3380,6 +3383,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
}

// stack effect: (__0, __array[oparg] -- )
Expand All @@ -3393,16 +3397,16 @@ dummy_func(
PyObject *list = SECOND();
DEOPT_IF(!PyList_Check(list), CALL);
STAT_INC(CALL, hit);
// CALL + POP_TOP
JUMPBY(INLINE_CACHE_ENTRIES_CALL + 1);
assert(_Py_OPCODE(next_instr[-1]) == POP_TOP);
PyObject *arg = POP();
if (_PyList_AppendTakeRef((PyListObject *)list, arg) < 0) {
goto error;
}
STACK_SHRINK(2);
Py_DECREF(list);
Py_DECREF(callable);
// CALL + POP_TOP
JUMPBY(INLINE_CACHE_ENTRIES_CALL + 1);
assert(_Py_OPCODE(next_instr[-1]) == POP_TOP);
}

// stack effect: (__0, __array[oparg] -- )
Expand All @@ -3420,7 +3424,6 @@ dummy_func(
PyObject *self = SECOND();
DEOPT_IF(!Py_IS_TYPE(self, callable->d_common.d_type), CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyCFunction cfunc = meth->ml_meth;
// This is slower but CPython promises to check all non-vectorcall
// function calls.
Expand All @@ -3438,6 +3441,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3454,7 +3458,6 @@ dummy_func(
PyObject *self = PEEK(total_args);
DEOPT_IF(!Py_IS_TYPE(self, d_type), CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
int nargs = total_args-1;
STACK_SHRINK(nargs);
_PyCFunctionFastWithKeywords cfunc =
Expand All @@ -3475,6 +3478,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3492,7 +3496,6 @@ dummy_func(
DEOPT_IF(!Py_IS_TYPE(self, callable->d_common.d_type), CALL);
DEOPT_IF(meth->ml_flags != METH_NOARGS, CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
PyCFunction cfunc = meth->ml_meth;
// This is slower but CPython promises to check all non-vectorcall
// function calls.
Expand All @@ -3509,6 +3512,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand All @@ -3526,7 +3530,6 @@ dummy_func(
PyObject *self = PEEK(total_args);
DEOPT_IF(!Py_IS_TYPE(self, callable->d_common.d_type), CALL);
STAT_INC(CALL, hit);
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
_PyCFunctionFast cfunc =
(_PyCFunctionFast)(void(*)(void))meth->ml_meth;
int nargs = total_args-1;
Expand All @@ -3544,6 +3547,7 @@ dummy_func(
if (res == NULL) {
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_CALL);
CHECK_EVAL_BREAKER();
}

Expand Down
Loading