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

compiler can incorrectly optimize a run of stores to the same name preceded by a SWAP #104615

Closed
carljm opened this issue May 18, 2023 · 3 comments
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error

Comments

@carljm
Copy link
Member

carljm commented May 18, 2023

If the apply_static_swaps optimization in the compiler sees the instruction sequence SWAP 2; STORE_FAST a; STORE_FAST a, it will optimize that by removing the SWAP and swapping the two instructions, resulting in STORE_FAST a; STORE_FAST a.

But of course, in this case the two instructions are identical, and their ordering matters because they store to the same location. So this change results in the wrong value being stored to a.

This was exposed by comprehension inlining, since it can result in this bytecode sequence for code in the form a = [1 for a in [0]] (where the first STORE_FAST a is restoring the previous value of a from before the comprehension, if any, and the second STORE_FAST a is storing the result of the comprehension to a.).

Linked PRs

@carljm carljm added the type-bug An unexpected behavior, bug, or error label May 18, 2023
@carljm carljm self-assigned this May 18, 2023
@carljm
Copy link
Member Author

carljm commented May 18, 2023

I see two options for fixing this:

  1. Change apply_static_swaps to also track store locations and consider instructions not swappable if they store to the same location.
  2. Add redundant store elimination, so prior to apply_static_swaps we would reduce SWAP 2; STORE_FAST a; STORE_FAST a to SWAP_2; POP_TOP; STORE_FAST a, which apply_static_swaps would correctly optimize to STORE_FAST a; POP_TOP.

Probably the ideal is to do both; (2) because it results in the best compiler output, and (1) because it is more robust and avoids implicit dependency of one optimization on another.

@brandtbucher
Copy link
Member

This is definitely a bug in apply_static_swaps:

3.10:

>>> def f(x, y):
...     a, a = x, y
...     return a
... 
>>> f(True, False)
False

3.11:

>>> def f(x, y):
...     a, a = x, y
...     return a
... 
>>> f(True, False)
True

@brandtbucher brandtbucher added 3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker labels May 18, 2023
carljm added a commit to carljm/cpython that referenced this issue May 18, 2023
carljm added a commit to carljm/cpython that referenced this issue May 18, 2023
@carljm
Copy link
Member Author

carljm commented May 18, 2023

I'm considering this issue fixed by the merged PR. I filed #104635 for the separate question of improving compiler output in these cases with dead store elimination.

@carljm carljm closed this as completed May 18, 2023
carljm added a commit to carljm/cpython that referenced this issue May 18, 2023
* main:
  pythongh-74690: Don't set special protocol attributes on non-protocol subclasses of protocols (python#104622)
  pythongh-104623: Update Windows installer to use SQLite 3.42.0 (python#104625)
  pythongh-104050: Add more type annotations to Argument Clinic (python#104628)
  pythongh-104629: Don't skip test_clinic if _testclinic is missing (python#104630)
  pythongh-104549: Set __module__ on TypeAliasType (python#104550)
  pythongh-104050: Improve some typing around `default`s and sentinel values (python#104626)
  pythongh-104146: Remove unused vars from Argument Clinic (python#104627)
  pythongh-104615: don't make unsafe swaps in apply_static_swaps (python#104620)
  pythonGH-104484: Add case_sensitive argument to `pathlib.PurePath.match()` (pythonGH-104565)
  pythonGH-96803: Document and test new unstable internal frame API functions (pythonGH-104211)
  pythonGH-104580: Don't cache eval breaker in interpreter (pythonGH-104581)
carljm added a commit to carljm/cpython that referenced this issue May 19, 2023
carljm added a commit that referenced this issue May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

2 participants