Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Dec 19, 2025

The optimization broke and causes segfaults now, but there were no tests so no one noticed. I added a test so we will catch it in the future.

@Fidget-Spinner
Copy link
Member Author

@corona10 I think this was introduced in the _CALL_TUPLE_1 refcount elimination work :(. Not your fault though, there were no tests for this optimization it seems!

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm!

value = PyStackRef_FromPyObjectBorrow(ptr);
}

tier2 op(_SWAP_CALL_ARG_LOAD_CONST_INLINE_BORROW, (ptr/4, callable, null, arg -- res, a, c)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you give this a name that describes what it does. It doesn't call anything, nor have an argument.
callable must be the builtin len function, so is immortal and can be dropped. Avoid the register shuffle:

Suggested change
tier2 op(_SWAP_CALL_ARG_LOAD_CONST_INLINE_BORROW, (ptr/4, callable, null, arg -- res, a, c)) {
tier2 op(_LOAD_CONST_INLINE_BORROW_SWAP4_DROP, (ptr/4, callable, null, arg -- res, n, a)) {
assert(_Py_IsImmortal(callable));

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Not quite done. You don't need to shuffle the values around as much.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I dont shuffle them the following POP_TOP will segfault?

Copy link
Member

Choose a reason for hiding this comment

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

It won't on the default build, as NULL is immortal. You'd need to check on the FT build.

@markshannon
Copy link
Member

Actually, let's not pop the callable, just declare it dead.
This is simpler and has less shuffling:

op(_CALL_LEN, (len_func, null, arg -- res, a)) {

 macro(CALL_LEN) =
      unused/1 +
      unused/2 +
      _GUARD_NOS_NULL +
      _GUARD_CALLABLE_LEN +
      _CALL_LEN +
      POP_TOP;

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 19, 2025

@markshannon len is not immortal, only on FT builds it is. We need to decref it otherwise there will be refleaks.

@markshannon
Copy link
Member

Oh, I assumed it was.

@Fidget-Spinner Fidget-Spinner merged commit 786f464 into python:main Dec 19, 2025
70 checks passed
@Fidget-Spinner Fidget-Spinner deleted the fix_const_folding_tuiple branch December 19, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants