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

fix decorator detection bug #50

Merged
merged 1 commit into from Sep 20, 2022

Conversation

15r10nk
Copy link
Collaborator

@15r10nk 15r10nk commented Sep 20, 2022

try to fix #49

I also reported frame.f_lasti and the problem is that the instruction is a CACHE instruction in the deco case (f_lasti == 130).

The Tester case gets the expected instruction and maps the bytecode correctly.

You can find my source here https://github.com/15r10nk/executing/tree/decorator-detection-bug

grafik

I could ignore CACHE instructions until now. I will try to figure out what they are good for.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 20, 2022

Logically, this space is part of the preceding instruction. Many opcodes expect to be followed by an exact number of caches, and will instruct the interpreter to skip over them at runtime.

using the real instruction before the CACHE seems to work

@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 20, 2022

@alexmojaki can I disable that the pipeline runs for every commit of a merge-request? I don't want to waste resources.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 20, 2022

This should fix the bug. I also added analyse.py which is very useful for debugging executing. @alexmojaki please tell me if it can stay there or should I move it somewhere else?

@alexmojaki
Copy link
Owner

can I disable that the pipeline runs for every commit of a merge-request? I don't want to waste resources.

When would it run for a PR then?

This should fix the bug.

Awesome, thanks! Do you understand the deal with CACHE? Do you think there should be a CPython issue?

I also added analyse.py which is very useful for debugging executing. @alexmojaki please tell me if it can stay there or should I move it somewhere else?

It looks great, let's keep it. Do you think putting it under the tests folder would make sense?

@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 20, 2022

I created a draft-pull request and did not expected the pipeline to run. It is not a big problem I can also use [skip ci] in the commit message.
I am also quiet new to github and I dont know how things usually work her.

CACHE is documented here. The documentation says only that it stores something and that it behaves like an NOP.

I think it is an issue because it is documented that the interpreter should step over CACHE instructions. Do you want to report it or should I?

@alexmojaki
Copy link
Owner

Please report it

@alexmojaki
Copy link
Owner

Is this PR still a draft?

…tion

also added analyse.py because it is really helpful for debugging.
@15r10nk 15r10nk marked this pull request as ready for review September 20, 2022 18:42
@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 20, 2022

created this bug report and added it as a comment to the work around python/cpython#96970.

no draft any more

@alexmojaki
Copy link
Owner

Thanks!

@alexmojaki alexmojaki merged commit 1164838 into alexmojaki:master Sep 20, 2022
@15r10nk 15r10nk deleted the decorator-detection-bug branch September 20, 2022 20:37
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.

3.11 decorator detection bug
2 participants