-
Notifications
You must be signed in to change notification settings - Fork 88
Various minor crash/etc fixes #22
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
base: master
Are you sure you want to change the base?
Conversation
(overrides previous fix, as merging them would be too complex and unneeded)
I was not able to get it to happen for pretty huge functions (much much larger than the ones in the test), though, so there's a good chance it's unreachable.
Thanks, very much appreciated! But reviewing this huge PR is difficult. Let's land #21 separately, and if you don't mind can you move the other changes that are unrelated of supporting Python 3.8 to separate pull requests as well? Also can you update |
(This was originally in the second PR, but since you want to take this one first, I think it's a good commit to add here, since it's relevant to this change and would make diffing against future PRs easier)
Added 4 tests, 3 currently disabled (pre-existing issues, especially on pypy).
(The new test helps test functionality that needs special attention at least on py38)
(Candidate for moving to 'fix' branch?)
Add new test_jump_out_of_try_block_and_live
Uses a weak dictionary just in case code objects can be gc'd (can they? maybe on some implementations?)
add warnings for easier debugging (for peace of heart - they weren't currently triggered by any tests)
(later fix could go into fix branch as well)
(No testcase in this branch, as it'd be tricky to think of one. master branch has testcases becaues it does dis.findlabels)
Plus added extra test from master
a86304e
to
ac926dc
Compare
I've split this PR into py38 and non-py38. (This is the non-py38 PR and should be merged AFTER the py38 PR) Splitting the PR further would be too much work, though, and it contains mostly related fixes (+ one unrelated minor change that should hopefully be easy to review together with the rest - just look at lines containing _patched_code_cache) Let me know if you're unsure about why something's been added or changed, I'll gladly explain. |
I didn't do a full review yet (as this pull request is difficult to review until rebased/merged against
|
(Change makes it so POP_BLOCK is no longer looked at (as it's a poor indicator in some cases), instead - we look at the target of each block. Also includes a simplification removing block_exits)
I've updated the changes per flake8 & your notes in the other PR. To respond to your questions above:
Since the decorator runs every time outer_func is called, we would needlessly patch the function's bytecode every time (E.g. imagine outer_func is called in a heavy loop).
Additional notes:
EDIT: I'll also do a refactor of _find_labels_and_gotos to make it use a class for the block_stack management, as it's currently quite brittle (especially due to the lack of python3.x's nonlocal) |
(Not possible in 3.8 and as comment says - it's not clear how pypy and py38 will reconcile their differences there)
While this isn't strictly required, making it happen requires fully handling the has_setup_except variation (replacing SETUP_FINALLY with SETUP_EXCEPT where appropriate) so it's a good thing to have in the repo for the future
About (2), while it's not possible to (normally?) detect this at runtime (since the generated opcodes are the same - their semantic is different), I realize I could largely remove the feature check altogether and always write the END_FINALLY when exiting finally/with blocks (previously - was only written for pypy). In fact, one could well say that not calling END_FINALLY in such cases is a bug, just one that non-pypy pythons don't care about (of course, one could say the same about adding goto to python in general, but let's not go there :P) Let me know if you'd rather have the pypy feature check over writing 2 extra opcodes (BEGIN_FINALLY/equivalent & END_FINALLY) in all relevant cases. The extra 2 opcodes shouldn't be a problem now that we can write how much we want. Also, I went ahead a bit further and made some changes to fully handle the _BYTECODE.has_begin_finally flag by translating SETUP_FINALLY to SETUP_EXECPT once detected as well. This allowed me to remove the pypy feature check entirely (as opposed to under the !has_begin_finally feature check). This PR is now ready from my perspective. Let me know if you want any changes. |
This includes pull request #21 and #24 and the following fixes (with tests):
It also includes the following non-fix enhancements:
All locally tested on Python 2.6/2.7/3.4/3.8 & pypy2.7&3.6