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

update pypcode to use version >2.0.0 #27

Merged
merged 3 commits into from
Mar 20, 2024
Merged

update pypcode to use version >2.0.0 #27

merged 3 commits into from
Mar 20, 2024

Conversation

RobinDavid
Copy link
Collaborator

Quokka was enforcing pypcode<2, but a new version 2.0.0 was released. This merge request updates the pypcode part to use the latest version. The following things have changed.

  • ContextObj is no longer exposed
  • Context.translate, returns a TranslationResult which sole attribute is directly a list of PcodeOps. No more instructions

This merge request changes the following:

  • change ContextObj to Context object.
  • remove combine_instructions() as translating a block returns directly a list of PcodeOps
  • remove the equality and object_hash monkey patching. Its was problematic to port, and not sure it was really useful ? And used for ? (ping @DarkaMaul )

Shall be prooftested before merging...

@DarkaMaul
Copy link
Collaborator

remove the equality and object_hash monkey patching. Its was problematic to port, and not sure it was really useful ? And used for ? (ping @DarkaMaul )

I think it was used to be able to create dict of instructions and/or allow them to be cached.

@patacca
Copy link
Collaborator

patacca commented Mar 12, 2024

I force-pushed to rebase it on the current main.
I also change the requirement from pypcode >= 1.1.1 to >= 1.1.2

Just FYI pypcode now is marked as an optional dependency (so install it with pip install quokka-project[pypcode])

@patacca
Copy link
Collaborator

patacca commented Mar 12, 2024

remove the equality and object_hash monkey patching. Its was problematic to port, and not sure it was really useful ? And used for ? (ping @DarkaMaul )

I think it was used to be able to create dict of instructions and/or allow them to be cached.

This is the never-ending dilemma about caching/uncaching 😅

I think at the moment there is still a bit of misalignment between what is supposed to be cached and what is not. In QBinDiff and python-binexport we adopted the design choice of having the functions preloaded, the basic blocks by default not cached and everything below basic block (instructions, operands, etc...) as cached by default.
If someone wants to access the basic blocks in a cached way they can either load/unload them manually or by using the python context manager (with statement).

Overall it seems like a good solution that we might enforce on quokka as well. This means also that we might need to reintroduce the equalty and hashing operations at some point.

Maybe we can create an issue as a remainder for enforcing this caching design choice and for now go on and not care about caching pcode objects.

@RobinDavid
Copy link
Collaborator Author

I think changes I made breaks the backward compatibility with older versions of pypcode. Thus we need to enforce pypcode>=2.0.0.

Was the dependency optional previously ? If not I would prefer to leave it as mandatory for ease of use.

@patacca
Copy link
Collaborator

patacca commented Mar 13, 2024

I think changes I made breaks the backward compatibility with older versions of pypcode. Thus we need to enforce pypcode>=2.0.0.

Ah sorry, I thought the requirement was for >= 1.1.2

Was the dependency optional previously ? If not I would prefer to leave it as mandatory for ease of use.

It has been optional since fe701ba

@RobinDavid
Copy link
Collaborator Author

Ready to be merged on my side.

@RobinDavid RobinDavid merged commit 3f9e327 into main Mar 20, 2024
13 checks passed
@patacca patacca deleted the upgrade-pypcode branch April 30, 2024 09:06
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 participants