Rust MMIO #1499
Rust MMIO #1499
Conversation
Previously the user data of MMIO callbacks would live until the end of the containing Unicorn engine. Now they are deallocated once all memory referencing those callbacks has been unmapped.
d3df235 to
a237505
Compare
|
Why not take a borrow instead of the ownership of the callbacks? Sorry I’m not a rust expert and my question may be naive. |
|
Mostly I tried to be consistent with the existing API for hooks where the closures are also moved instead of borrowed. I'm not very versed in rust either, so I'm not completely sure what the reason for that is. But I think it's something like this: If we want to borrow the closures for use in the C code, we have to borrow it with a lifetime that is valid for as long as the C code might use it. The C code might use the MMIO callbacks as long as any memory is mapped that references them. I don't think this last constraint can be expressed by rust's borrowing rules. Lifetimes are always associated with scopes, but the information if some memory is still mapped does not correspond to any scope. The closest fit we could use, is to borrow the closures with the same lifetime as the By taking ownership of the closures, we can be more concise. We handle the deallocation of the closures explicitly, which is internally unsafe (because it would violate the borrowing rules if we used borrowing instead of a pointer), but we provide a safe interface over it that ensures that the closures live as long as the C code might use them, but is still able to free them as soon as they are not needed anymore. |
Nice explanation. Keeping consistent with exiting API makes sense to me. Taking ownership is also more flexible later. |
I noticed that the rust bindings apparently don't have a way to map MMIO regions at the moment. This PR adds
map_mmioto provide this functionality.Some things that I'm unhappy with, but don't see a way to make them better at the moment (feedback very welcome):
mmio_mapto take a pointer to the user data and then move theBoxes of the user data intoself.inner_mut().mmio_callbacksonly works, because the pointer isn't checked by regular borrowing rules. This is basically the same method as used for hook callbacks and AIUI theBoxguarantees that the contents of the box will always stay in the same location, so this should be safe. But I would be slightly more satisfied with a solution that would also work under normal borrowing rules.MmioCallbackScope.unmapis kinda complicated and easy to get wrong. I'm not really confident that I didn't make any off-by-one errors. Unfortunately I'm also not really sure how to properly write automated tests for this, because the test suite doesn't have access toUnicorn.inner()to check that deallocation is actually working as intended.Unicorn<D>.mmio_unmapcould probably be made more efficient by keeping theMmioCallbackScopes sorted and using some sort of binary search to find regions to unmap. However, I wanted to keep this implementation simple for the moment. The introduced logic is already complicated as-is and I would imagine thatmem_unmapisn't very performance critical for most use cases. But if you think a more performant implementation is needed, I can take a look at it again.