gh-114058: Only allow immortal constants in tier 2 #115817
Conversation
Fidget-Spinner
changed the title
| @@ -317,7 +310,7 @@ sym_new_known_type(_Py_UOpsAbstractInterpContext *ctx, | |||
| return res; | |||
| } | |||
|
|
|||
| // Takes a borrowed reference to const_val. | |||
| // Decrefs const_val if it is not immortal and discards it. | |||
There was a problem hiding this comment.
This seems like an interface that is hard to use. Let the caller worry about mortality.
This also contradicts the comment on sym_new that it is OK to pass in mortal objects as long as the caller retains a strong reference.
There was a problem hiding this comment.
That way is more complex to handle error conditions.
E.g
For newly created objects, out of space means we should decref it, but the code isn't as clean as
OUT_OF_SPACE_IF_NULL(res = sym_new_const(ctx, temp));
but rather becomes
if (res = sym_new_const(ctx, temp) == NULL) {
Py_DECREF(temp);
goto out_of_space;
}
Then we need to repeat that on every call site.
There was a problem hiding this comment.
Either temp is immortal, in which case the caller need do nothing, or it is mortal in which case it needs proper handling.
Correctness is paramount here, as bugs are going to be very hard to reproduce. Reproducing bugs in the T1 specializer is hard enough. We need to extra careful here. A bit of verbosity is a small price to pay for being able to reason about the code.
There was a problem hiding this comment.
The proposed code here would totally invalidate _BINARY_OP_ADD_FLOAT and other float-producing uops (as well as int-producing uops if the value is not a cached short int -- which is a distinction easily overlooked when testing).
| // Same as sym_new_const, but borrows const_val, assuming it will be kept alive. | ||
| // Only safe if we know const_val has a strong reference elsewhere. | ||
| static inline _Py_UOpsSymType* | ||
| sym_new_const_borrow(_Py_UOpsAbstractInterpContext *ctx, PyObject *const_val) |
There was a problem hiding this comment.
I think you should scrap this, and leave it to the caller to handle lifetimes.
|
When you're done making the requested changes, leave the comment: |
|
All commit authors signed the Contributor License Agreement. |
This PR bans all non-immortal constant propagated values, except for values that have a strong reference guaranteed (like _LOAD_CONST_INLINE).