search

unicorn-engine / unicorn

Unicorn CPU emulator framework (ARM, AArch64, M68K, Mips, Sparc, PowerPC, RiscV, S390x, TriCore, X86)
http://www.unicorn-engine.org
GNU General Public License v2.0
7.34k stars 1.31k forks source link

Implement reg_{read,write}2 API #1835

Closed nneonneo closed 1 year ago

nneonneo commented 1 year ago

As suggested in #1831, this patch implements a "version 2" API for register read/write which takes a size parameter. Context and batch operations are all supported.

This patch also comes with some performance optimizations and some (slightly opinionated) refactoring. The performance improvement appears to be about 20-25% for a single read/write operation or about 10% for a batch operation of size 8, despite the addition of the size parameter.

As a side benefit, the register APIs now produce an appropriate error if an invalid (unknown) register is used.

Fixes #1831.

wtdcode commented 1 year ago

Look good to me in general though ideally your fixes and code format should be splitted into smaller commits. The work is pretty nice and complete!

However, we also need to port this to new bindings like python, rust and your java bindings. But this is out of this PR's scope and I don't have further concerns.

nneonneo commented 1 year ago

However, we also need to port this to new bindings like python, rust and your java bindings. But this is out of this PR's scope and I don't have further concerns.

I think that we would not expose these bindings to Python or Java directly (maybe Rust though). Rather, we would just have Python/Java always call the new APIs for safety, since they will always know how big their input buffers are.

wtdcode commented 1 year ago

However, we also need to port this to new bindings like python, rust and your java bindings. But this is out of this PR's scope and I don't have further concerns.

I think that we would not expose these bindings to Python or Java directly (maybe Rust though). Rather, we would just have Python/Java always call the new APIs for safety, since they will always know how big their input buffers are.

Yes, that's my point. We should further update bindings to use the new API.

nneonneo commented 1 year ago

Any news on this PR?

wtdcode commented 1 year ago

Any news on this PR?

Rushing on a conference deadline ;(

Will have a look once getting it done.

wtdcode commented 1 year ago

I'm done with my deadlines and will soon start to review your two PRs (and probably other stale ones XD).

So far, for this one, I don't see any further problems. Thanks in advance!

nneonneo commented 1 year ago

Rebased to latest dev, and changed UC_ERR_NOMEM to a new error UC_ERR_OVERFLOW - hope that's reasonable?

wtdcode commented 1 year ago

Merged, thanks for your big contributions!