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

Support additional API on Python 3 bindings #2016

Merged
merged 12 commits into from
Oct 6, 2024
Merged

Conversation

elicn
Copy link
Contributor

@elicn elicn commented Sep 29, 2024

Highlights:

  • Redesign certain architecture-specific aspects to better leverage OOP
    • Code de-duplication
    • Easier to extend or support new architectures
  • Now fully support reg_write_batch and reg_read_batch API
  • Introduced coprocessor register accessors to AArch32 and AArch64, named cpr_read and cpr_write
  • 1 ARM regression test fixed
  • More comments, documentation and annotations

def reg_write(self, reg_id: int, value) -> None:
# select register class for special cases
reg_cls = UcAArch32.__select_reg_class(reg_id)
assert check_maxbits(coproc, 4)
Copy link
Member

@wtdcode wtdcode Oct 6, 2024

Choose a reason for hiding this comment

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

Why not make cpr_read/write raise an UC_ERR_ARG? Terminating the whole process without a chance to catch seems too aggressive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally speaking, exceptions are thrown when the arguments normally make sense but there is a specific problem this time (e.g. opening a file which happens to be inaccessible, etc.). In these cases the user may catch the exception and choose how to handle it.

Here the assertions come to block an invalid usage with incorrect arguments: it is not a call that may sometimes work and sometimes not -- rather it is invalid. There is no way to "handle" such case other than re-writing that piece of code.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I also forget AssertionError can be caught so let it go.


def msr_read(self, msr_id: int) -> int:
return self._reg_read(const.UC_X86_REG_MSR, UcRegMSR, msr_id)
return self.reg_read(const.UC_X86_REG_MSR, msr_id)

def msr_write(self, msr_id: int, value: int) -> None:
Copy link
Member

@wtdcode wtdcode Oct 6, 2024

Choose a reason for hiding this comment

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

This seems the same as above, which should raise an exception, but maybe next time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not following on this one. What should raise an exception? It marks this line:

return self.reg_read(const.UC_X86_REG_MSR, msr_id)

@wtdcode wtdcode merged commit ac4872b into unicorn-engine:dev Oct 6, 2024
33 checks passed
@elicn elicn deleted the more-api branch October 6, 2024 15:20
wtdcode pushed a commit that referenced this pull request Oct 13, 2024
* Styling and commets fixes

* Add errno API support

* Improve OOP approach by adjusting the way reg types are selected

* Leverage new approach to deduplicate reg_read and reg_write code

* Adjust reg_read_batch

* Add support for reg_write_batch

* Adjust x86 MSR accessors

* Turn asserts into descriptive exceptions

* Improve comments and styling

* Fix ARM memcpy neon regression test

* Modify canonicals import

* Introduce ARM CP reg accessors
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.

2 participants