Skip to content
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.

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

GH-128682: Stronger checking of PyStackRef_CLOSE and DEAD. #128683

Merged
merged 5 commits into from
Jan 13, 2025

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jan 9, 2025

Stronger checking of the stack state for PyStackRef_CLOSE. Only allow references to be closed from top to bottom, to ensure stack is valid when closing a reference.

Fix up bytecodes.c accordingly.

@markshannon markshannon marked this pull request as ready for review January 10, 2025 15:39
@markshannon markshannon changed the title GH-128682: Fix interaction of DEAD and DECREF_INPUTS GH-128682: Stronger checking of PyStackRef_CLOSE and DEAD. Jan 10, 2025
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

My impression was that, rather than enforcing a closing order, we would instead have the code generator insert NULL assignments to the stack where necessary. I vaguely remember situations where that would be really useful, such as when pushing or popping multiple stack items that are created or freed in the "wrong order", or where overlapping lifetimes would force us to convert things to heap references (and back again) to move things around on the stack.

Are we abandoning that idea, or is this a step towards that?

(Either way, the code in this PR seems correct.)

int total_args = oparg;
_PyStackRef *arguments = args;
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind this alias? I see we do this in a couple of places now...

If the reasoning is that the analysis chokes on array accesses, maybe we should teach it to ignore them?

Copy link
Member Author

Choose a reason for hiding this comment

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

We modify arguments, but the code generator assumes that args is a fixed array on the stack.

@@ -116,7 +116,7 @@ def __init__(self, out: CWriter):
"SAVE_STACK": self.save_stack,
"RELOAD_STACK": self.reload_stack,
"PyStackRef_CLOSE": self.stackref_close,
"PyStackRef_CLOSE_SPECIALIZED": self.stackref_close,
"PyStackRef_CLOSE_SPECIALIZED": self.stackref_close_no_escape,
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to have this one "inherit" it's escaping-ness from its second arg? I could see us adding something for tuples or whatever and breaking this by accident.

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

@markshannon
Copy link
Member Author

My impression was that, rather than enforcing a closing order, we would instead have the code generator insert NULL...

That only applies when several references are removed from the stack at once, by DECREF_INPUTS.

If references are popped individually, they must be popped in the right order.

@markshannon markshannon merged commit 517dc65 into python:main Jan 13, 2025
63 checks passed
@markshannon markshannon deleted the escaping-decref-1 branch January 13, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants