gh-104615: don't make unsafe swaps in apply_static_swaps#104620
Uh oh!
There was an error while loading. Please reload this page.
Conversation
carljm commented May 18, 2023
One thing I'm not clear on is whether we want to bump bytecode magic number for a fix like this. The old bytecode still works as well it ever did (no incompatible changes in the interpreter), so maybe no? But OTOH, nobody will see this bugfix until their pycs are invalidated. Basically: do we bump bytecode magic number on any change to the compiler that results in different compiler output, or only when we change the interpreter in a way that makes it incompatible with previous bytecode? |
JelleZijlstra commented May 18, 2023
I was wondering a bit about that too. Several other recent fixes around comprehensions would also change the compiler output, so theoretically we should have bumped the magic number a few times. On the other hand, the only people who are really affected would be people who built from main directly after the PEP 695 change was merged (which did bump the magic number), so realistically there's not much of an issue. It's probably enough if we bump the magic number one last time before the beta is finalized. |
carljm requested review from brandtbucher, iritkatriel and markshannon as code owners
My thought has mostly been that magic numbers should only be bumped if the bytecode itself is incompatible (meaning opcodes or the meanings of opargs have changed). I don't think we've bumped it for miscompilations before (but I could be wrong). (We also bump it when changing how code objects are unmarshalled, which is arguably an abuse of the magic number and should be a marshal version bump instead.) |
| } | ||
| for (intidx=j+1; idx<k; idx++){ | ||
| intstore_idx=STORES_TO(block->b_instr[idx]); | ||
| if (store_idx >= 0&& (store_idx==store_j||store_idx==store_k)){ |
There was a problem hiding this comment.
How would it happen that there's a STORE_FAST* between j and k?
There was a problem hiding this comment.
This can happen with code like a, a, b = x, y, z as in some of the added tests. This compiles to LOAD_FAST x; LOAD_FAST y; LOAD_FAST z; SWAP 3; STORE_FAST a; STORE_FAST a; STORE_FAST b. Before this PR, apply_static_swaps would optimize that (ignoring the loads) to STORE_FAST b; STORE_FAST a; STORE_FAST a (swapping the first and third store), which is invalid because it reorders the two stores to a.
k - j == n - 1 here, where n is the oparg to the SWAP. So for a SWAP 2, j and k will be adjacent, but for SWAP with oparg > 2, there will be intervening instructions. And STORE_FAST is a SWAPPABLE instruction, so those intervening instructions can be STORE_FAST.
carljm commented May 18, 2023
Oh, I forgot: this should be backported to 3.11, since it's a bug that exists there as well. |
miss-islington commented May 18, 2023
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
miss-islington commented May 18, 2023
Sorry @carljm, I had trouble checking out the |
carljm added needs backport to 3.11 miss-islington commented May 18, 2023
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
miss-islington commented May 18, 2023
Sorry, @carljm, I could not cleanly backport this to |
* 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)
…pythonGH-104620). (cherry picked from commit 0589c6a) Co-authored-by: Carl Meyer <carl@oddbird.net>
bedevere-bot commented May 19, 2023
GH-104636 is a backport of this pull request to the 3.11 branch. |
6 participants
It's not safe to apply SWAP statically if it would reorder two instructions that store to the same location.