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

Abstract Virtual Machine Support in byterun #12

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

Conversation

arthurp
Copy link

@arthurp arthurp commented Aug 4, 2014

I used byterun as the basis for an abstract VM for some work here at Google and I wanted to offer the changes back to you. This also fixes some bug, notably classes work correctly after the first commit. However the bulk of the changes are to allow abstract interpretation to be done.

-Arthur Peters (working from inside Google)

Arthur Peters added 6 commits July 31, 2014 10:45
Fix several bugs to make unit tests pass:
* Make data stack local to the frame
* Implement classes and inheritance properly (incl. method resolution order)
* Provide called functions their own local dictionary in the appropriate cases
The method resolution was implemented using Python License code derived from https://www.python.org/download/releases/2.3/mro/. This introduces a new license, but avoids reimplementation of a core algorithm.
Simplify some interfaces, and add a bunch of functions that can be
overridden to replace the concrete behavior with abstract behavior.
Add a CFG class that converts CPython code objects into a rough CFG form.
Add AbstractVirtualMachine class and AncestorTraversalVirtualMachine class that provide features useful for abstract interpreters.
…ACK_SEQUENCE, slices, and imports.

Make reverse_operator_name only reverse operators that have a reverse.
Fix tests so they work in the exported version of the codebase.
Fix tests that were failing when exported via moe due to hardcoded line numbers.
Update README.google.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=72433464
…. Otherwise this would need parallel maintenance for no real gain.

Add setup.py from opensource project so that it can be changed along with other files in google CL if needed.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=72441558
Conflicts:
	byterun/pyvm2.py
@akaptur akaptur mentioned this pull request Aug 4, 2014
@arthurp
Copy link
Author

arthurp commented Aug 4, 2014

Oh I should mention something here. I have only tested this on PY2.7. Since that's my target here. I did make an attempt to stay portable, but I'm sure I messed it up somewhere. So a PY3 fix commit will probably be in order. I should be able to produce that in a few weeks if needed (once I'm done with my google internship, so I have more time).

@akaptur
Copy link
Contributor

akaptur commented Aug 4, 2014

@arthurp , are you set up using tox? It makes the cross-version testing much less of a headache

@arthurp
Copy link
Author

arthurp commented Aug 4, 2014

I don't have tox setup yet. However I will when I have time to devote to this. I am aware of it. Thanks for the reminder.

@makmanalp
Copy link

Spectator note: If you guys set up with travis, I think they process your tox.ini to run the tests on all platforms.

@nedbat
Copy link
Owner

nedbat commented Aug 5, 2014

@arthurp This is amazing! Thanks for all this work.
Reviewing this would be easier if we could have separate PRs for the various aspects, though I understand that's difficult to accomplish sometimes.
BTW: setting up tox is dead simple (well, once you have Python 3.3 installed also)

$ pip install -r requirements.txt
$ tox

@@ -17,3 +17,53 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.


PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this license text?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I copied the MRO algorithm code directly from the Python documentation which is under this license.

@nedbat
Copy link
Owner

nedbat commented Aug 5, 2014

@arthurp Could you tell us more about what you used this for in Google? It will help us understand the motivation for the refactoring.

@@ -105,6 +105,7 @@ def run_python_file(filename, args, package=None):
main_mod.__file__ = filename
if package:
main_mod.__package__ = package
# TODO(ampere): This may be incorrect if we are overriding builtins
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could expand these TODO's into GitHub issues, we have a better chance of fixing them in the future. For example, do you know under what circumstances this is incorrect?

@arthurp
Copy link
Author

arthurp commented Aug 5, 2014

A apologize for the giant PR and big commits. It's partly just a bad habit of mine (doing everything and then pushing), but also an artifact of trying to make sure that every internal google commit is testable. I don't think I have a good way to take this apart at this point. Though if github supports it I might well be able to push each of my 3 main commits as a separate PR which would help somewhat.

Would you be willing to glance at the commits and tell me if that would be useful? It would really get weird in terms of the commit history it would produce I think, so I don't want to do it unless it's useful.

About the project: This is related to the previous google project here pytypedecl (https://github.com/google/pytypedecl). The idea is to produce a type language for python. This work with byterun is to create a system that can do type inference on python program. An abstract interpreter is a good way to do static analysis, hence all the abstraction, so I can replace each operation with abstract versions that track the information we are interested in.

-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=72568656
@nedbat
Copy link
Owner

nedbat commented Aug 10, 2014

@arthurp I think I'd like to do this: generalize byterun so that it can be subclassed as you want, but let's keep the specific subclasses out of the byterun repo. So for example, your Control Flow Graph could be a separate repo that uses and subclasses byterun's classes.
What should we expect from you in terms of working more on this pull request to get it to a common understanding?

@arthurp
Copy link
Author

arthurp commented Aug 12, 2014

This makes a lot of sense. Just so I understand: You want to integrate my changes to pyvm2, but I should lift abstractvm and other things into a dependent project. I can do that but probably not for another few weeks. I'm in my last weeks at good and I'm quite busy.

But once I get started: Do you want to minimize the changes to pyvm2? Or directly abstract it? I think it would be possible to push a lot of the changes I made in pyvm2 into abstractvm. However it might increase code duplication. What would be your preference?

@nedbat
Copy link
Owner

nedbat commented Aug 13, 2014

@arthurp yes, you have the right idea. I'm not sure what changes from pyvm2 could go into AbstractVm, but I definitely don't like "increased code duplication". Take a stab at the best way you think, and we can go from there.

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

4 participants