gh-104615: don't make unsafe swaps in apply_static_swaps #104620
Conversation
|
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? |
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,
markshannon and
iritkatriel
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 (int idx = j + 1; idx < k; idx++) { | ||
| int store_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.
|
Oh, I forgot: this should be backported to 3.11, since it's a bug that exists there as well. |
|
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Sorry @carljm, I had trouble checking out the |
carljm
added
needs backport to 3.11
|
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
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>
|
GH-104636 is a backport of this pull request to the 3.11 branch. |
It's not safe to apply SWAP statically if it would reorder two instructions that store to the same location.