Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-104615: don't make unsafe swaps in apply_static_swaps#104620
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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. |
brandtbucher commentedMay 18, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How would it happen that there's a STORE_FAST* between j and k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can happen with code likea, a, b = x, y, z as in some of the added tests. This compiles toLOAD_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) toSTORE_FAST b; STORE_FAST a; STORE_FAST a (swapping the first and third store), which is invalid because it reorders the two stores toa.
k - j == n - 1 here, wheren is the oparg to theSWAP. So for aSWAP 2,j andk will be adjacent, but forSWAP with oparg > 2, there will be intervening instructions. AndSTORE_FAST is aSWAPPABLE instruction, so those intervening instructions can beSTORE_FAST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the quick fix!
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 |
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 commit0589c6a)Co-authored-by: Carl Meyer <carl@oddbird.net>
bedevere-bot commentedMay 19, 2023
GH-104636 is a backport of this pull request to the3.11 branch. |
Uh oh!
There was an error while loading.Please reload this page.
It's not safe to apply SWAP statically if it would reorder two instructions that store to the same location.