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

Add ANTLR4 grammar and C++ parser #28

Merged
merged 8 commits into from Dec 11, 2020

Conversation

HackerFoo
Copy link
Contributor

@HackerFoo HackerFoo commented Dec 3, 2020

This PR replaces the textX pearser with a C++ implementation using ANTLR to improve parsing speed 16x: 80 seconds down to 5 seconds, parsing the LiteX 35T fasm with this:

import fasm

for line in fasm.parse_fasm_filename('top.fasm'):
    pass

@mithro mithro requested a review from litghost December 3, 2020 01:08
Makefile Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mithro mithro left a comment

Choose a reason for hiding this comment

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

As you are adding C++ code this repo, you should also set up our normal set of C++ compliance checks (I believe there are copies in the prjxray repository?).

cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
fasm/FasmLexer.g4 Outdated Show resolved Hide resolved
fasm/FasmParser.g4 Outdated Show resolved Hide resolved
fasm/__init__.py Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
@mithro
Copy link
Contributor

mithro commented Dec 3, 2020

This PR replaces the textX pearser with a C++ implementation using ANTLR to improve parsing speed 30x.

Can you give some more concrete numbers?

@litghost
Copy link
Collaborator

litghost commented Dec 3, 2020

Should delete textx schema?

@mithro
Copy link
Contributor

mithro commented Dec 3, 2020

What about tests?

cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
cpp/ParseFasm.cpp Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
fasm/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Overall looks like a good start. Relatively simple C++ -> Python boundry. I think we are missing some internal state assertions on the Python side, and I've tried to comment on them when I see them. What do you think about having more focused unit tests on the Python and C++ side?

@litghost
Copy link
Collaborator

litghost commented Dec 3, 2020

@HackerFoo Please transition the CI from Travis to GH actions in a separate PR. It will significantly aid in CI runtime.

@HackerFoo
Copy link
Contributor Author

What about tests?

I've been using tests/test_simple.py. I'll need to look into creating a GitHub action to run it.

@litghost
Copy link
Collaborator

I've tested the fallback, seems to work as expected.

If the fallback is used during building, how does the user know why the fallback was used? Should there be a warning on pip install?

I've figured out how to get the error, running pip install -v fasm will show the error during install. Please add that information to the fallback warning, so that users can determine why the fallback implementation is being used. Specifically, if the user uninstalls and re-installs the package, e.g.:

pip uninstall fasm
pip install -v fasm
# or
pip install -v git+https://github.com/HackerFoo/fasm.git@antltr_cpp
# or
pip install -v git+https://github.com/Symbiflow/fasm.git

Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Can you add an argument to fasm/tool.py to select the parse? Similar to what you've done in test_simple.py?

@litghost
Copy link
Collaborator

@HackerFoo, review is in.

Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
@HackerFoo
Copy link
Contributor Author

@litghost I've addressed all of your comments, thanks.

@litghost
Copy link
Collaborator

litghost commented Dec 10, 2020

@litghost I've addressed all of your comments, thanks.

Yep, things are looking pretty good. I noticed something wrong when uninstalling the package.

Try this, with ANTLR4 runtime installed:

python3 -mvenv test_env
source test_env/bin/activate
pip install -v git+https://github.com/HackerFoo/fasm.git@antltr_cpp
pip uninstall fasm

I get the following message when running pip uninstall fasm:

pip uninstall fasm
Found existing installation: fasm 0.0.2
Uninstalling fasm-0.0.2:
  Would remove:
    test_pip/bin/fasm
    test_pip/lib/python3.7/site-packages/fasm-0.0.2-py3.7.egg-info
    test_pip/lib/python3.7/site-packages/fasm/*
  Would not remove (might be manually added):
    test_pip/lib/python3.7/site-packages/fasm/parser/libparse_fasm.so
    test_pip/lib/python3.7/site-packages/fasm/parser/tags.py
Proceed (y/n)? 

Note the lines under Would not remove (might be manually added):. Even if I say y, it doesn't remove the .so and tags.py. Do you know why?

Copy link
Collaborator

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Please fix package uninstall not removing all parts of the package.

@HackerFoo
Copy link
Contributor Author

HackerFoo commented Dec 10, 2020

@litghost It doesn't look like there's a way to fix this presently. Here's the code responsible for deciding what to skip:

https://github.com/pypa/pip/blob/master/src/pip/_internal/req/req_uninstall.py#L167-L215

It won't remove anything without a *.pyc extension.

From https://www.activestate.com/resources/quick-reads/how-to-uninstall-python-packages/

Be aware that there are a few exceptions that cannot be uninstalled with pip, including:

Distutils packages, which do not provide metadata indicating which files were installed.
Script wrappers installed by the setup.py develop command.

GitHub
The Python package installer. Contribute to pypa/pip development by creating an account on GitHub.
ActiveState
The Pip Package Manager is the de facto standard for managing Python distributions, and can be used to uninstall Python packages.

@litghost
Copy link
Collaborator

tags.py should definitely be able to be removed. Is the issue that the relevant files are missing from the manifest / metadata?

@HackerFoo
Copy link
Contributor Author

I just tried it. Adding them to MANIFEST.in has no effect. I think MANIFEST.in is only meant for source files.

@HackerFoo
Copy link
Contributor Author

I think this is just how pip uninstall is meant to work. I can add an issue for it.

Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
@HackerFoo
Copy link
Contributor Author

HackerFoo commented Dec 11, 2020

@litghost I've created an issue to track this: #37

@litghost
Copy link
Collaborator

@litghost I've created an issue to track this: #37

Please fix this. The solution took me 15 minutes of research to find, which is that you need to supply add pyproject.toml in the root with the following content:

[build-system]
requires = ["setuptools", "wheel"]

Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
@HackerFoo
Copy link
Contributor Author

@litghost I've created an issue to track this: #37

Please fix this. The solution took me 15 minutes of research to find, which is that you need to supply add pyproject.toml in the root with the following content:

[build-system]
requires = ["setuptools", "wheel"]

@litghost Thanks, this works.

Could you please include where you found this solution on #37 for future reference? Thanks.

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

3 participants