Skip to content

Remove the size limit for memory read and write (revamped) #2144

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

Merged

Conversation

rliebig
Copy link

@rliebig rliebig commented Apr 5, 2025

This PR is a reimplementation of PR #1799 made by @secretnonempty, is made mainly created for technical reasons:

  • The old PR has been stale for two years and solving two years worth of merge conflicts seems disproportional effort
  • My rights on this repository do not premit me to make any changes to the old PR

I do not want to attribute to any of the great work done by @secretnonempty, I am simply interested in having these changes in the upcoming release.

Some modifications had to be made, some of these were necessary to reflect the changes done during the stale period.

Please be aware that the originial test made by @secretnonempty allocates a least 4GB of memory, which can strain some systems and might invoke a SIGKILL if there is not enough memory available. The writing operations inside the large write operations are also preformed consecutively, I think yielding the thread somewhere in between the write operation could be useful for large writes to avoid system freezes.

I also added the handling to the Zig and Haskell bindings.

Zig seems only to correctly build with version 0.13.0, but not 0.14.0 Currently, the main release branch of Ziglang is at 0.15.0, althrough my interest in that part is somewhat limited. These changes are related to Zig Issue 20178, so I took some liberty to adjust it to work, so I can at least test these.

I might also add a new test case for the specific problem, which sent me done this specific rabbit hole. It was related to a large call for mprotect.

I really would like some tests for these different kind of language bindings, seeing as I had to touch quite a bit of code over several files.

@rliebig
Copy link
Author

rliebig commented Apr 5, 2025

Oh, I just noticed the github workflow for zig. We might need to extract the modifications to the build.zig.zon file then. The action included for Zig is goto-bus-stop setup-zig, which recently has become unmaintained.

@wtdcode
Copy link
Member

wtdcode commented Apr 6, 2025

Before starting a review, I would like to know if it is a full re-implementation? If you still borrow some code from him, I will add Co-Author-By to git message when accepting the PR.

@rliebig
Copy link
Author

rliebig commented Apr 6, 2025

Please add @secretnonempty as a coauthor. I simply modified his changes to the current state of the project, which has somewhat changed due to some refactorings. I will start wrangling with the pipeline. As introduced in the PR text, the android system sadly reached an out of memory condition, which resulted in a kernel panic. That will be fun to mitigate.

@wtdcode
Copy link
Member

wtdcode commented Apr 6, 2025 via email

@rliebig
Copy link
Author

rliebig commented Apr 6, 2025

@wtdcode I believe I am there. CI/CD Tests are passing and manual tests for my use case work.

@wtdcode
Copy link
Member

wtdcode commented Apr 7, 2025

The quality of the PR is rather high and I appreciate your efforts. I don't find anything to comment at this moment and I think this can be accepted as it is. However, I need to deal with #2138 firstly so that the following PR won't conflict with that.

By the way, if I understand it correctly, it looks like it only introduces break changes on 32 bits platforms where size_t is not uint64_t?

@rliebig
Copy link
Author

rliebig commented Apr 7, 2025

I believe it is slightly more complicated: QEMU natively supports int64 values(as hwaddr, for example) and the current bindings reduced it to size_t and inside the further language bindings down to uint32. This caused, as mentioned inside the #1799, that if the RAM memory exceeds more than 4GB , or if addresses higher than addressable with 32bit integers to throw a UC_ERR_ARG exception, instead of processing the command. So to speak: the current API maimed the QEMU API in this regard. This MR simply alignes these again to uint64 values.

Now, in the case of 32bit size_t, I believe there should be no issues. The change affects 64bit platforms as well, because the bindings now allow access to higher memory ranges.

@wtdcode
Copy link
Member

wtdcode commented Apr 7, 2025

I understand the semantics and the issues to fix of both pull requests. What I said "break" only refers to the API signatures of our current C API defined in unicorn.h.

@rliebig
Copy link
Author

rliebig commented Apr 7, 2025

Sorry, my bad! Yes, that should be the case.

@wtdcode
Copy link
Member

wtdcode commented Apr 12, 2025

Hello, we have adopted #2138 . Could you fix the conflicts? Once that is done, I think it’s ready to get merged.

Thanks in advance!

@rliebig
Copy link
Author

rliebig commented Apr 13, 2025

Sure, will ping you when I am Done!

@rliebig
Copy link
Author

rliebig commented Apr 13, 2025

@wtdcode at least on my end CI/CD tests are passing - could you run them here please?

@wtdcode wtdcode merged commit 455aae2 into unicorn-engine:dev Apr 15, 2025
44 checks passed
@wtdcode
Copy link
Member

wtdcode commented Apr 15, 2025

Thanks and welcome!

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