Description

carljm

opened on May 18, 2023

Issue body actions

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

• gh-104615: don't make unsafe swaps in apply_static_swaps #104620
• [3.11] gh-104615: don't make unsafe swaps in apply_static_swaps (GH-104620). #104636

Activity

1. carljm

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   on May 18, 2023
2. carljm

   self-assigned this

   on May 18, 2023
3. carljm commented on May 18, 2023

   

   carljm

   on May 18, 2023

   MemberAuthor

   More actions

   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.
4. brandtbucher commented on May 18, 2023

   

   brandtbucher

   on May 18, 2023

   Member

   More actions

   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

   Reacted by Carl Meyer
5. brandtbucher

   added

   3.11only security fixesonly security fixes

   3.12only security fixesonly security fixes

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   release-blocker

   on May 18, 2023
6. ezio-melotti

   added this to Release and Deferred blockers 🚫on May 18, 2023
7. github-project-automation

   moved this to Todo in Release and Deferred blockers 🚫on May 18, 2023
8. carljm

   added 3 commits that reference this issue on May 18, 2023

   pythongh-104615: don't make unsafe swaps in apply_static_swaps

   5028eda

   pythongh-104615: don't make unsafe swaps in apply_static_swaps

   22533ba

   gh-104615: don't make unsafe swaps in apply_static_swaps (#104620)

   0589c6a
9. carljm commented on May 18, 2023

   

   carljm

   on May 18, 2023

   MemberAuthor

   More actions

   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.

   Reacted by Jelle Zijlstra
10. carljm

    closed this as completedon May 18, 2023
11. github-project-automation

    moved this from Todo to Done in Release and Deferred blockers 🚫on May 18, 2023
12. carljm

    added 3 commits that reference this issue on May 18, 2023

    Merge branch 'main' into nestedcomps

    54095f0

    [3.11] pythongh-104615: don't make unsafe swaps in apply_static_swaps (…

    eb83a26

    [3.11] gh-104615: don't make unsafe swaps in apply_static_swaps (GH-1…

    667e4ec

Issue actions