Skip to content

[3.13] gh-133441: Fix STORE_ATTR_WITH_HINT bytecode #133446

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

Merged
merged 5 commits into from
May 11, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented May 5, 2025

Deoptimize if the dict is a dict subclass.

Deoptimize if the dict is a dict subclass.
@vstinner
Copy link
Member Author

vstinner commented May 5, 2025

cc @ZeroIntensity @sobolevn

@ZeroIntensity
Copy link
Member

It looks like the offending code is on main as well.

@vstinner
Copy link
Member Author

vstinner commented May 5, 2025

It looks like the offending code is on main as well.

I failed to write code to trigger the bug on main, so I'm not sure that main is affected.

@@ -1594,6 +1594,24 @@ def make_pairs():
self.assertEqual(d.get(key3_3), 44)
self.assertGreaterEqual(eq_count, 1)

def test_store_attr_with_hint(self):
# gh-133441: Regression test for STORE_ATTR_WITH_HINT bytecode
Copy link
Member

Choose a reason for hiding this comment

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

This should be in test_opcache not test_dict IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the test to test_opcache, but I'm not sure if TestInstanceDict is a good home for such test.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Thanks!

@vstinner
Copy link
Member Author

vstinner commented May 9, 2025

@Fidget-Spinner: Do you think that the main branch is also affected? I'm unable to trigger the bug in the main branch, but the code looks the same. I would prefer to not deoptimize just for an hypothetical case if it cannot occur in practice. The code in the main branch is a little bit different.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented May 9, 2025

@vstinner did you try wrapping the repro in a loop? Sometimes that might trigger it. I mean an actual Python for loop, not python -m test -F

@vstinner
Copy link
Member Author

vstinner commented May 9, 2025

@vstinner did you try wrapping the repro in a loop? Sometimes that might trigger it. I mean an actual Python for loop, not python -m test -F

I just tried to put the reproducer in a loop: I still cannot reproduce the failure on the main branch.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM as well. If 3.14 and 3.15 aren't affected, someone should add the test case.

@vstinner vstinner merged commit 5cd56b2 into python:3.13 May 11, 2025
56 of 57 checks passed
@vstinner vstinner deleted the store_attr_with_hint branch May 11, 2025 21:10
@vstinner
Copy link
Member Author

Merged, thanks.

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