Skip to content
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

Various minor crash/etc fixes #22

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

insignification
Copy link

@insignification insignification commented Dec 13, 2019

This includes pull request #21 and #24 and the following fixes (with tests):

  • Fixes for jumping out of try/with/except/finally statements (esp. in pypy and now-supported Python 3.8)

It also includes the following non-fix enhancements:

  • Cache patched code blocks to avoid unnecessary patching when @with_goto is applied on a nested function.
  • Add warnings if something goes wrong for easier debugging. (If you get a warning when applying with_goto, there's a very good chance the resulting function will misbehave or crash)

All locally tested on Python 2.6/2.7/3.4/3.8 & pypy2.7&3.6

insignification and others added 2 commits December 10, 2019 23:48
(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.
@snoack
Copy link
Owner

snoack commented Dec 13, 2019

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 tox.ini, .travis.yml and README.md in order to cover Python 3.8?

condut and others added 17 commits December 14, 2019 01:27
(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)
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)
@insignification insignification changed the title Fixes for various crashes & Python 3.8 support Various minor crash/etc fixes Dec 14, 2019
@insignification
Copy link
Author

insignification commented Dec 14, 2019

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.

@snoack snoack mentioned this pull request Dec 15, 2019
@snoack
Copy link
Owner

snoack commented Dec 15, 2019

I didn't do a full review yet (as this pull request is difficult to review until rebased/merged against master) but I already have a couple remarks:

  1. I'm not a fan of the caching. It seems like a premature optimization and a potential memory leak. But I also fail to see a common scenario where the same code is passed through the goto decorator more than once.
  2. Specifically detecting __pypy__ seems wrong to me. I rather stick to feature detection (e.g. compile some code, and inspect the resulting bytecode, like I did in order to detect Python 3.6+ wordcode, see the _Bytecode class and how its properties are initialized).

condut added 3 commits December 15, 2019 21:58
(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)
@insignification
Copy link
Author

insignification commented Dec 15, 2019

I've updated the changes per flake8 & your notes in the other PR.

To respond to your questions above:

  1. The common scenario that this handles is as follows:
def outer_func():
        @with_goto
        def inner_func():
            <do something func with gotos>
        return inner_func

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).
This results not only in needless speed loss but also in needless creation of duplicate code objects.
This is why caching is required and is not a premature optimization.
As for a memory leak - that's what using a weak-keyed dictionary should resolve. (Supported in py2.7+). Plus, code objects usually have infinite scope, so it's not a real concern even in py2.6 (compared with the alternative).

  1. Hmm, that could work - I'll give it a try.

Additional notes:

  1. I added two "noqa"s - one for the pypy import (will probably be removed per (2)) and one for a test checking the "except:" case (I moved most tests to check the "except Exception:" case, but keeping one good test for the "except:" case is important too.

  2. Due to changing the "except" to "except Exception" in the testcases, I ended up finding a new issue. To resolve it, I made some additional code changes, including "backporting" a relatively major change from the next PR - code no longer relies on POP_BLOCK but instead looks at the jump argument supplied in the SETUP_* opcodes, which seems more reliable in several cases.
    (To be clear, the "backported" changes are relevant to this PR - the fixing of various crashes/etc in existing supported python-goto functionality)

  3. Let me know if you want me to make the following improvement to the code:
    Use namedtuple instead of the brittle ,,x=y and y[0] approaches. (Supported in py26+ so should be good)

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)
EDIT EDIT: Done.

condut added 4 commits December 15, 2019 23:35
(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
@insignification
Copy link
Author

insignification commented Dec 15, 2019

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).
It's not strictly required, but fits better with the "you should call END_FINALLY after finally/with blocks" narrative and will be helpful if we ever want to add other changes that rely on SETUP_EXCEPT.

This PR is now ready from my perspective. Let me know if you want any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants